Post-response task running (destructable services) are actually blocking; add test coverage and warn for common misconfiguration

Now needs a rebase for 10.1.x.

So an interesting observation here re: BigPipe; when you are using mod_php BigPipe will flush its response to the client as expected, however because it sends no content-length header at the outset (because it doesn't know the content-length) the client doesn't close the connection after everything is sent. So the client appears to continue to "spin" during post-request processing. If you close the client connection, the server side continues to do its post-request tasks because:

PHP will not detect that the user has aborted the connection until an attempt is made to send information to the client. (source)

This is kinda annoying UX in that the client doesn't immediately close and give the perceived performance of the page being "done" loading. However if the client disconnects, PHP keeps going on its merry way because no further output is attempted to be sent. I had thought perhaps we needed to implement ignore_user_abort(true) but I think this means, no we don't. This is one annoying feature of big_pipe when you can't call fastcgi_finish_request() but, whatever.

That BigPipe behaviour might be changed by 📌 Try to use either Transfer-Encoding: chunked or a real streamed response for BigPipeResponse Needs work - probably wouldn't affect the post-response tasks but might/should help with the spinny wheel issue.

Re: #63:

Instead of using sleep() this could just be something like setting a state flag in the test and then resetting it in the last destructable service, and polling for that change in the test, no?

I thought we could do that, and that was my initial implementation of the test... however because this is a browser/functional test, the service is not accessible from the test runner; it's running in the context of the tested site. Really tough.

I think it would be helpful if those who are making commits (and opening new MRs?) could explain their work? Also it would be helpful for the work to stay on the main MR that has been worked on (2568) since its target has been changed to 11.x... no excuse not to stay there.

MR 2568 is now rebased to 11.x, hoping to maintain the MR review history in one place. Tests should still be passing as evidenced by the other 11.x MR that was created so I think this should be ready for another pass by @catch.

larowlan → closed merge request !4345

larowlan → closed merge request !2509

We've been using this patch in prod for 6+ months and it's working well. Also seems that all feedback/discussions have been addressed.

This looks good to go now! Great work everyone!

This all looks great.

Committed c469089 and pushed to 11.x. Thanks!
Hopefully see people over on 📌 Try to use either Transfer-Encoding: chunked or a real streamed response for BigPipeResponse Needs work

For potential follow-up: Post-response tasks will still be blocking in mod_php if the response is deflated by mod_deflate. The only way I can think to resolve this would be for the page cache to do its own compression of the response, so that mod_deflate doesn't come into play.

There's follow-up ✨ Add a way to delay executions in test runner until terminate event completed in the child site Fixed

Since when this was committed, lots of functional tests started failing with deadlock errors when running some test-groups over Github Actions on my experimental database driver DruDbal.

See here an example - https://github.com/mondrake/drudbal/actions/runs/5653538436

Adding to the test build a patch that reverts this commit makes tests succeed again. See https://github.com/mondrake/drudbal/actions/runs/5753586671.

This is just to say that not necessarily just the tests covered by fixes in ✨ Add a way to delay executions in test runner until terminate event completed in the child site Fixed could fail, but many others and that it seems something that impacts functional testing overall.

Note - I'm not asking for a reversal here, rather looking forward a fix for FunctionalTest overall.

@mondrake does your test suite run with READ_COMMITTED? This seems like the kind of error we get with REPEATABLE_READ.

#87 I also thought READ COMMITTED might have been the root cause, since DruDbal was behind core in adopting it, and therefore enabled it in https://github.com/mondrake/drudbal/commit/7d89e517e870c4f7e71de39f8bb90.... The number of failures reduced after that, but ATM the only way to have a full green run is to somehow patch core with a reversal of this issue.

Anyway, let's not get crazy about DruDbal, it's what it is i.e. an experiment. I will be looking at the follow-up and trying it with DruDbal as well.

@mondrake please try to use percona:5.7.22 or mysql:5.7.22 image instead of mysql:5.7

For me this is the last version where the most of custom projects are stuck to be deadlock free

Looks it needs separate issue to explore deeper

@andypost thanks. Tried that in PR https://github.com/mondrake/drudbal/pull/333, still gets to deadlock errors. Anyway, MySql 5.7.43 was fine before this issue went in, and is fine again with a patch that is reverting it. So for me we should really need to see in the followup what other cases are there where post-response tasks in the SUT are blocking test tasks.

I run tests on Github Actions with vanilla Drupal 11.x and MySql 5.7.43 and they pass OK. So it must be something with DruDbal transaction management, that was undercover and this issue just surfaced. It’s on me then. Sorry for the noise.

This introduced a new random testfailure 🐛 [random test failure] DisplayFeedTranslationTest::testFeedFieldOutput Fixed .

Per #3379199-7: [random test failure] DisplayFeedTranslationTest::testFeedFieldOutput → the follow-up just committed in ✨ Add a way to delay executions in test runner until terminate event completed in the child site Fixed fixes #92, so I think any regression talk should get followed-up there. FWIW I don't think any of us were particularly in love with the sleep() calls on this issue so it's awesome to see a creative fix to harden this (still new) test coverage.

Automatically closed - issue fixed for 2 weeks with no activity.

Change records published.

Reroll for 10.1.x, there were some conflicts with dictionary.txt

This appears to have broken the batch system, if an operation callback throws an exception. See #3392196-12: Exceptions in batch no longer are shown on the page → .

I think this change needs to be reverted. There are the reasons explained above but also there are other things about the content-length header documented in https://www.rfc-editor.org/rfc/rfc9110.html#name-content-length that are not accounted for. Among these are:

• For HEAD requests content-length should be equal to the equivalent GET request - tested this and it works so that's good.
• 1xx and 204 cannot have a content length header
• 304s has a complex rule about being the same as a 200

I also think there are some other things we should account for based on the issues we had when Symfony set a content-length header on everything. See https://github.com/symfony/symfony/issues/46449.

• No content-length header when the transfer-encoding header is set.
• No content-length header when $response->getContent() returns FALSE.

Furthermore, I think this change is inherently risky because we're relying on the content-length we set here being correct but as we can see from the issues linked above post commit this is definitely not the case. It is highly likely that this change will be disruptive so I think we need to make a bigger noise about it in whichever release it lands.

Also, reading https://www.drupal.org/node/3298550 → makes it sound like Drupal 10.2.0 will ship with a performance penalty in certain configurations due to this change. Which is not the case. This change has the possibility to improvement performance in relation to 10.1 if you enable output buffering.

We need a revert MR here since the original commit doesn't revert cleanly (although it looks like it should be easy to resolve) and there have been test follow-ups since (although mostly those should be no-op changes without this MR).

Generally I think we'd be fine restricting the content length headers to 200, 403 and 404 since that's the only user-facing responses we actually care about.

After discussing with @catch a bit I've opened 🐛 Only set content-length header in specific situations Fixed to try to go forward here instead of a revert.

I just wanted to note that we ran into an issue with New Relic after adding the Content-Length header. It wouldn't inject the js-agent into the html because there was a Content-Length header (probably because it changes the length by doing so). So, we stopped getting PageView information until we removed the header.

https://docs.newrelic.com/docs/release-notes/agent-release-notes/php-rel...

One work around is to add the script tag manually in code. That way you get the correct Content-Length and you don't rely on it being injected automatically by the php agent.

https://docs.newrelic.com/docs/apm/agents/php-agent/features/browser-mon...

Just saw this message in some installations and I'm still curious, what the php.ini / ini_set value should look like.

Could we perhaps add an example to the message in the status report or at least in the Change record?
Linking the php.net website is nice, but doesn't help a lot to tell the user what exactly to do and not to introduce even worse misconfigurations. Not all Drupal Users are Server Administration professionals, but they see the message and will try to get it away...

https://www.drupal.org/project/tailwindcss_utility → inlines the tailwind css via a http_middleware service. But that gets invoked _after_ this call. Leading to errors like, "upstream sent more data than specified in "Content-Length" header while reading upstream" from nginx. Apache works just fine but nginx doesn't like it. Opening up a follow-up to see what we can do. Maybe the contrib module should adjust things, but alternatively, couldn't this be implemented in a middleware as well?

Added 🐛 Regression from #3295790 content-length header set earlier than expected Fixed and a patch to start the conversation. Then I found 🐛 Only set content-length header in specific situations Fixed , which is going a different direction. Not sure which to suggest at the moment. At the very least, we should expand on implications in the change record. While working on the patch, I found an issue with Cors that on the surface would break anyone using Cors and Nginx right now in 10.x without any use of contrib modules.