[stable9.1] [debug] Debug fclose wrapper #26735
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@PVince81, thanks for your PR! By analyzing the history of the files in this pull request, we identified @DeepDiver1975, @tanghus and @lenz1111 to be potential reviewers. |
|
Weird, I wanted to paste my results but |
PVince81
force-pushed
the
stable9.1-fclose-wrapper-debug
branch
from
7dbb01e to
b9782eb
Compare
|
Ok, I've adjuted the PR to limit the trace length. Apparently this makes it log something. In my local env where the issue did not appear, here is how the trace looks like when downloading a public link file: |
|
Nevermind my last comment, it was a typo. |
|
|
@Webbeh was there no matching "in callback" entry ? Is this from a case where the |
|
Oh @Webbeh last time when you added |
|
Yes I added fclose() everywhere it was missing inside /lib, therefore inside readfilePart too. It was only using the "download" button, no video player. |
|
In callback coming |
|
@Webbeh which browser ? I was testing with Chromium 54 with the Download button. and now... WTF ?! Looks like PHP is GC'ing resources from previous calls even in unrelated calls... the heartbeat doesn't do anything! |
|
Firefox developer (aurora) 51.0a2 |
|
Apparently... Can't access \OCP\Util:writeLog from inside the callback... weird. |
|
I mailed myself the backtrace instead :
Received a few of them, they're all the same, even for different files. |
|
Yes, that's another static method. I think it's a variant of the original bug but specific to this PR/patch. Looks like the failing trace in your case starts directly in the View method in the callback. I'm more and more suspecting a PHP 7 GC bug and will do more research in that direction. @Webbeh which PHP version do you have ? I have 7.0.7. In my test case it always uses Now I also wonder: if the download is finished early, usually the browser might decide to close the network connection early. In some cases this could lead to the PHP process to get killed, so maybe here it wouldn't reach I'll add more logging inside readfile and readfilePart so we can see if it even reached fclose. |
|
PHP 7.0.13. Should I remove all the fclose() I added ? I also have readfile and not only readfilePart. ` ` |
|
I should maybe mention that I'm using REDIS as memcache.locking and memcache.local, not sure if it changes anything. |
PVince81
force-pushed
the
stable9.1-fclose-wrapper-debug
branch
from
b9782eb to
9af2e7d
Compare
|
Ok, first things first, let's find out why I've updated this PR to now log something before and after |
|
Do you have a good online JSON formatter ? EDIT : separated requests. Still not received any "in callback" yet. |
-> Still the same. |
|
@Webbeh I usually use CLI It seems your log didn't contain any message about "preparing to fclose" or "fclose called". I got lucky again locally, this is what I got: In short, they're all from the same request here:
This seems to tell us that Then why is it calling the callback a second time ? Maybe PHP is calling I'll do more testing. If that's the case, we might be able to check for duplicate |
|
I have no occurence of close yet... |
|
@Webbeh so you're only getting "before loop" but never after loop. I'm using mod_php btw. Are you using php-fpm ? (I seem to remember that the latter was more brutal regarding to killing) |
|
I actually have no idea, but I think it's mod_php7. phpinfo() doesn't give any sign of having FPM but lists mod_php7. |
|
Confirmed mod_php. |
|
From http://php.net/manual/en/features.connection-handling.php
I've updated the patch to call |
|
Hah, I think I got an idea for a trick: basically don't send the last block to the client yet and keep it until after I'll first wait for your results to see if the theory matches and then can try it out. |
|
Nope, no fclose even after ignore_user_abort() for big files. They do appear for files that can be downloaded before the "undeclared static property" error appears. |
|
@Webbeh can you precise whether it's going to readfile or readfilePart when you do see fclose ? Are you using any download manager in Firefox ? I'm still surprised that it does range requests at all. Or maybe the default download manager downloads multiple pieces at once ? (like download managers often do to speed up download) |
|
No download manager. Comes with readfile. |
|
@Webbeh are you able to try again with the last commit ? |
|
PVince81
commented
Dec 1, 2016
| @@ -420,16 +420,27 @@ public function filesize($path) { | |||
| * @throws \OCP\Files\InvalidPathException | |||
| */ | |||
| public function readfile($path) { | |||
| ignore_user_abort(); | |||
There was a problem hiding this comment.
I forgot to set this to true... let's try it in the next-next run
|
|
@davitol and I managed to recreate this issue on two separate envs where On my side I used a script to close the connection too early during a download: #22370 (comment) So far adding |
|
Steps How I reproduced the issue. Testing enviroment: owncloud-enterprise-complete-9.1.3RC1.tar.bz2 ubuntu_lamp_1604:latest PHP 7.0.8-0ubuntu0.16.04.2
|
|
Closing as we have a solution |
|
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This will likely fill your logs so use with caution.
For every fopen and fclose it will log the stack trace.
Needs "loglevel => 0" in config.php.
Goal is to find out why adding
fcloseinreadfiledoesn't work.Steps:
@Webbeh