Unification of drupal_exit() and drupal_page_footer() and performance boost for FPM enabled environments

Tested with:
PHP-5.3.10 / FPM / Nginx / With zlib.output_compression / FF / Chromium: OK
PHP-5.3.10 / FPM / Nginx / Without zlib.output_compression / FF / Chromium: OK

As per RobLoach suggestion, added docblocks for posterity and comprehension.

Hum, after doing some research I found out about fastcgi_finish_request() which actually handles the performance side of this. In current Symfony head, they are handling it seamlessly in the Response::send() method.

May be we could move this code side by side with this one, and suggest a PR to Symfony (then we would be able to drop our own support for this and have a nicer result).

Before doing this, we need to get this patch commited anyway because we can't let PHP warnings happen.

Thanks.

After some reading (a lot) there is no clean way with a vanilla PHP to detach the process and continue in the background without an explicit Connection: Close HTTP headers (which is probably a bad thing to do for the sake of keep-alive).

We can still improve what I done by adding a flush(); call just after the while() loop: this triggers a real and early buffer send (without detaching PHP from the HTTPd), and allow a finer buffer handling for layer uppers (HTTPd, Proxies, etc...). That's what we should PR to SF guys. I don't have a lot of time right now so not doing it, but we should keep that in mind.

Output buffering and connection handling: there is magic

In Drupal, there is only one code path: we deliver a response. This was already true in Drupal 7, this is explicitely true in Drupal 8 since we have the Response object. This is always true, whether we deliver an error page (error response), not found page (which is a special case of the error one), a file, a redirection, or a normal page.

Now in PHP side of things everything will be sent directly into the standard output it is attached too, until we start the output buffer. But in some cases, there is actually magic, especially when enabling the PHP buffer gziping (on some environments) but I suspect that PHP could create itself other layers of buffers depending on the environment it is running in. Regarding connection handling, this is a bit more complex, you can read the PHP documentation as a start: http://php.net/manual/en/features.connection-handling.php

We also know that PHP will flush itself all opened output buffer when the script exits, which actually doesn't mean the browser will close the connection (for example, in Apache context, conection handling is handled by Apache, not PHP, which allows it to handle the HTTP keep-alive while that in any CGI gateway use case, it will close the connection to the gateway, leaving the HTTPd upper handling another connection by itself to the client, which is not the PHP connection).

Opening ourself an output buffer is the most sane thing to do, we need it as it is explained there it will improve performances when we render our templates.

Now please consider reading that page: http://developer.yahoo.com/performance/rules.html/ There is there a special paragraph about Flush the Buffer Early. I think that Drupal 7 attempts to achieve this by calling ob_flush(), but this is wrong. The only way to ensure the buffer is really flushed is calling the flush() method (see both documentations in http://www.php.net/manual/en/function.ob-flush.php and http://www.php.net/manual/en/function.flush.php

In cases where we are in CGI gateway, for exemple, we already have two connections layers upper, which means that buffer bloat is easy, and response time will increase if keep our own buffer back while proceding to shutdown.

Drupal 7 attempts to flush the buffer early using ob_flush(), but it actually doesn't push it to the upper layer (HTTPd or CGI gateway): this piece of code is particularely useless by itself, plus, it doesn't take into account that we might have multiple output buffers since PHP can in real life magically open some behind our backs.

The "undocumented magic" was (1) we open the buffer when started generating the page, (2) we flush it at the end. Very magic.

Yes, magically, not that there is any magic in it, but because we can't try to extrapolate all possible uses cases people will use Drupal in. So, first answer to you, there is real magic here in our standpoint (even though we all agree, magic does not exist).

Drupal 7 stable is broken

There is another point to consider: we should not leave errors pass to the client once the full page is rendered, for many reasons. First, some of those errors could be insignificant and happen, for exemple, during some non essential shutdown operations. Plus, errors coming into the buffer once all the HTML is rendered could break rendering. And last but not the least, we could send errors into redirect response, which may break them (and that is bad). Note that we will eventually log all errors in the watchdog, whether or not we display those, so it's a safe thing to turn off the output buffering at this point.

Ensuring that buffers are not just only flushed, but closed will avoid those errors to get back to it, in order to ensure that, we need to use ob_flush_end() instead of ob_flush().

Now considering the last two paragraphs and the fact that PHP can magically add output buffers behind our backs, that Drupal 7 code is useless because the ob_flush() is inneficient and broken because we could potentially send errors after the full page rendering thus breaking it that we have to fix it. We just have to replace the ob_flush() by:

while (0 < ob_get_level()) {
  ob_flush_end();
}

Flush the Buffer Early

Because we want to improve performances, we need to proceed to the real flush() call, so the code above might be improved by writing:

while (0 < ob_get_level()) {
  ob_flush_end();
}
flush();

Which does the trick and ensures that the full buffer is pushed to the upper layer, ensuring that the gateway we use (either direct HTTPd or CGI) will manage its own buffer instead of waiting for us to have finished our userspace shutdown operations to finish and thus decreasing the response time in all cases, no exception.

Now, we also now that FPM is quite used now, hopefully we can force an early connection release by calling the fastcgi_finish_request(), so Drupal 7 code should be fixed as this:

if (function_exists('fastcgi_finish_request')) {
  fastcgi_finish_request();
}
else {
  while (0 < ob_get_level()) {
    ob_flush_end();
  }
  flush();
}

If you don't like the actual commited comment, please feel free to quote this post into comments into the code above for the next patch.

All this algorithm is the actual result of actual one full day of working and testing, and I will have monday loading stress tests results too which will confirm or infirm, or non be non significant. Whatever those tests will say, we have either to remove this broken and useless (and confusing) Drupal 7 code, or fix it as expressed above.

Drupal 8 and Symfony 2

Symfony 2 doesn't seem to care about all other environments than FPM, since they just call the fastcgi_finish_request() if exists, but I think they are wrong about this one and their code could be improved. Anyway, their buffer release code is into the Response object, at the very end of the send() method, right after the class specific implementation content sending (which might be only HTTP headers). This means that like us, they will do it in every code path, their framework is supposed to always send a response.

This also mean they actually do it even earlier than us!

We should PR them for adding the exact same code as I exposed above, but in their response object. That way, we would not need to keep our own implementation and leave the onTerminate listener alone for normal shutdown operations, not buffering control, which would be cleaner.

Last answer

At the minimum, let's fix the documentation, as this is just completely false:

I demonstrated upper that is only partially false. Actually only and the HTTPd might release the connection sooner is wrong. We should write something like this: Using ob_end_flush() instead of ob_flush() will close the output buffer. This means that potential later errors won't get to the user. Flushing the buffers followed by the flush() call will force buffers to be pushed upper layer (HTTPd or CGI gateway) allowing it to send it to the client faster.

So we committed this without even diving into this, looking which code paths are affected and where we don't start an output buffer while maybe we should? Oh, and without any tests?

Flushing all the buffers at the kernel terminason and before doing additional shutdown operations is generally a good idea and exactly what most frameworks do, even SF does with FPM, just earlier than us: I don't see where additional tests could have actually helped us here. The only problem here is that SimpleTest actually did not catched our PHP warnings, and that in a sense is broken.

Here is the D8 proper patch.

And the D7 proper patch.

This could also be backported to D6.

@#18 It's fixed in #12 I said this part was wrong. If you read my 2 patches you will see I fixed it in both.

EDIT:

A five minute research should have convinced you of that.

Half an hour of reseach actually convinced me that it can be done using some messy tricks playing with HTTP headers to force the client to close the connection for us. That's why I wrote #8 -some hours before yours- and #12. In FPM context, the fastcgi_finish_request() actually closes the gateway connection. I played with both quite a bit, and even if making the hacky solution is quite easy, I'm sure this is something we don't want to do because we would mess up with other because playing too deep with HTTP connection handling headers may interact with upper layers. I think I don't need the 5 minutes you suggest anymore.

There is very much zero magic involved in this process.

There is some underlaying PHP magic under, read #12. Drupal 7 stable code is both broken and useless. EDIT: Not all Drupal 7, only the ob_flush() call, of course.

About Drupal 7 patch I made: there is still something to fix, because the drupal_session_commit() might attempt to send the session cookie to the user, we need the session to be started if needed before the flush() call.

Because the session commit is a costy task, I'd suggest to still doing it after the flush. We can still do that: testing if $_SESSION is empty, starting the session if not, then flush() then delaying the session commit after that should work fine. I have testing code on a patched site actually that works nicely.

I will update the patch soon.

A while ago I PR'ed SF2 https://github.com/symfony/symfony/pull/4914 and this piece of code evolved since, because some people managed to find out some bugs I missed until it reached https://github.com/symfony/symfony/issues/5033 all of those being merged.

We now need to revert current D8 patch, and need to fix the D7 version using the SF2 current algorithm, and we we then may be able to call this a win.

Please link to the patch, I spotted other regressions than just ob_flush()'ing, I want to be sure they are fixed.

Thanks, I will review this as soon as I can.

Actually, I went for a round of debugging recently to ensure we don't miss at least the lazy cookie send before we close the output buffer, and it seems that it works. I'd like to have some other pair of eyes verifying this, because in the earlier version of the patch this actually was one of the drawbacks.

I'd also like, if the D8 version is OK, to attempt a Drupal 7 clean port for this, case in which it'd loose the WSCCI tag, but for this the D7 maitainers need to give the go.

Sounds the right thing to do.

Yes there is.

Is there a solution for d7?
Ok, patch to be made. I did an ugly one for a custom project, I'll do a proper one for here.

Here is a D7 version (seems to be working on my local box) finger crossed that tests pass.

Switching back to D7 since the fix is applied de facto into Symfony's Symfony\Component\HttpFoundation\Response::send() method so applied per default to D8.

Fails seems to be irrevelant to the patch. I will try some debugging when I'll have some time for it.

#36: 1651010-35-better_flush-d7.patch queued for re-testing.

Ok maybe fails are related...

Retrying.

I think I might found some of the errors cause: the session commit must happen after the hook_exit() run. Did some improvements in documentation and added credits to copy/pasted Symfony code (this code is actually being used in Drupal 8).

Hope this new patch passes.

I have absolutely no idea why the update tests are failing.