Observed very regularly in the logs of translatewiki.net after updating from 910c6d8 2013-04-02 11:04:11 +0000 to f9ce826 2013-04-03 07:37:56 +0000
Version: 1.22.0
Severity: normal
URL: https://gerrit.wikimedia.org/r/#/c/57227/1/includes/resourceloader/ResourceLoader.php
Krinkle / Hoo: As you wrote / reviewed the patch that likely created this regression, could you take a look please?
Related URL: https://gerrit.wikimedia.org/r/58429 (Gerrit Change Ida0e48931781be77327073dc4ed3966949f4b082)
davidt-mediawiki-bugz wrote:
This change means I'm now seeing the 20-byte empty gzip bodies with 304 responses that this code is supposed to prevent.
I don't understand how it was supposed to work for ob_get_level() not equal to 0 or 1.
Consider the case where ob_get_level() starts at 2: ob_end_clean() will only be called once.
for ( $i = 0; $i < ob_get_level(); $i++ ) {
ob_end_clean();
}
$i = 0;
$i < ob_get_level(); [0 < 2: true]
ob_end_clean();
$i++ [$i = 1]
$i < ob_get_level(); [1 < 1: false]
Marius: As Timo is probably pretty busy with VE things today, mind taking a look at this issue?
Change 74570 had a related patch set uploaded by Hoo man:
Suppress notices while deleting output buffers
(In reply to comment #5)
This change means I'm now seeing the 20-byte empty gzip bodies with 304
responses that this code is supposed to prevent.
I've been wondering what those 20-byte content-length were all about. I spotted it here and there over the past few weeks (wasn't really looking for content-length, but I sort-of noticed it in the corner of my eye when looking at something else).
Nice catch.
(In reply to comment #7)
Change 74570 had a related patch set uploaded by Hoo man:
Suppress notices while deleting output buffers
Can you explain why caching the level is not a problem now? Aside from the warning suppression, this is exactly what I did a few months ago (back then just to optimise the code, not to fix anything in particular) and then I had to revert it as it caused this bug (bug 46836) and added an inline code comment about it, which you now removed.
In some (or many?) cases the level seems to go to zero in 1 call. So caching doesn't seem like a good choice.
(In reply to comment #5)
This change means I'm now seeing the 20-byte empty gzip bodies with 304
responses that this code is supposed to prevent.I don't understand how it was supposed to work for ob_get_level() not equal
to
0 or 1.Consider the case where ob_get_level() starts at 2: ob_end_clean() will only
be
called once.for ( $i = 0; $i < ob_get_level(); $i++ ) {
ob_end_clean();}
$i = 0;
$i < ob_get_level(); [0 < 2: true]
ob_end_clean();
$i++ [$i = 1]$i < ob_get_level(); [1 < 1: false]
So it should just be <= instead of <, right?
(In reply to comment #9)
So it should just be <= instead of <, right?
No. The problem is that $i increases while get_level() decreases at the same time. This is why caching get_level() changes the behavior to something that's arguably more correct.
Probably the best way to do this would be something like
while ( ob_get_level() > desired value ) { ob_end_clean(); }
Problem is I don't know enough about output buffering offhand to know what the desired value is. I *think* it's 0. Maybe it's 1. Who knows. Output buffering is magical.
(In reply to comment #10)
(In reply to comment #9)
So it should just be <= instead of <, right?
No. The problem is that $i increases while get_level() decreases at the same
time.
Right, it's obvious now that you pointed it out :)
(In reply to comment #11)
Is there a reason wfClearOutputBuffers() isn't used here?
Looks like a good candidate at first sight.
HISTORY shows that this was introduced/updated for a similar use case:
- (bug 8148) Handle non-removable output buffers gracefully when cleaning buffers for HTTP 304 responses, StreamFile, and Special:Export. Duplicated code merged into wfResetOutputBuffers() and wfClearOutputBuffers()
OutputPage::checkLastModified
$this->getRequest()->response()->header( "HTTP/1.1 304 Not Modified" );
wfClearOutputBuffers();
Patch in comment 7 got merged, not closing as I guess there is more work to do here according to comment 11.
(In reply to comment #13)
Patch in comment 7 got merged, not closing as I guess there is more work to
do
here according to comment 11.
The patch (which uses wfClearOutputBuffers mentioned in comment 11 now) hasn't been merged yet... if it gets merged I guess we can close this bug.
(In reply to comment #0)
Observed very regularly in the logs of translatewiki.net after updating from
910c6d8 2013-04-02 11:04:11 +0000 to f9ce826 2013-04-03 07:37:56 +0000
Per Nikerabbit, they no longer appear in recent logs.