Page MenuHomePhabricator

PHP Notice: ob_end_clean(): failed to delete buffer zlib output compression in includes/resourceloader/ResourceLoader.php on line 602
Closed, ResolvedPublic

Description

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

Details

Reference
bz46836

Related Objects

View Standalone Graph
This task is connected to more than 200 other tasks. Only direct parents and subtasks are shown here. Use View Standalone Graph to show more of the graph.

Event Timeline

bzimport raised the priority of this task from to High.Nov 22 2014, 1:38 AM
bzimport set Reference to bz46836.
siebrand created this task.Apr 3 2013, 7:54 AM

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.

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]

greg added a comment.Jul 15 2013, 5:08 PM

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.

hoo added a comment.Jul 22 2013, 11:04 AM

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