Wikimedia Bugzilla is closed!

I left a comment on 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)

Patch from comment 3 was merged.

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

https://gerrit.wikimedia.org/r/74570

(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
> 
> https://gerrit.wikimedia.org/r/74570

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.

Is there a reason wfClearOutputBuffers() isn't used here?

(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.

Patch still waiting for merge...

(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.

Change 74570 merged by jenkins-bot:
Use wfResetOutputBuffers in ResourceLoader

https://gerrit.wikimedia.org/r/74570

*** Bug 61359 has been marked as a duplicate of this bug. ***