Problem/Motivation

#3295790-34: Post-response task running (destructable services) are actually blocking; add test coverage and warn for common misconfiguration requires a fix present in 4.4.44 and 6.1.3 (10+), released 7/29/22.

list of branches are https://github.com/symfony/symfony/pull/46931#ref-pullrequest-1322183804

Priority to match PP'd issue.

Proposed resolution

Update lockfile and requirement.

Remaining tasks

Regression testing.

User interface changes

no

API changes

None breaking per Symfony BC contract.

Data model changes

no

Release notes snippet

Core require symfony/http-foundation 4.4.44+ and 6.1.3+

Issue fork drupal-3300773

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

bradjones1 created an issue. See original summary.

bradjones1’s picture

Version: 9.5.x-dev » 10.0.x-dev
Issue tags: +Needs backport to 9.x

bradjones1’s picture

Status: Active » Needs review

andypost’s picture

updated IS and remaining packages will get update via #3295520: Update dependencies for Drupal 10.0 and 9.5

and requeued

21:40:33 In Process.php line 1335:
21:40:33                                                                                
21:40:33   The process "sudo -u www-data git -C '/var/lib/drupalci/workspace/jenkins-d  
21:40:33   rupal_patches-140940/source' fetch origin  merge-requests/2567/merge:merge-  
21:40:33   request-2567" exceeded the timeout of 300 seconds.    
bradjones1’s picture

There's a bit of a chicken-and-egg situation on this - the one failure is relating to the addition of a vary header, which is to be expected with the new flush() done by the library. This is addressed in #3295790: Post-response task running (destructable services) are actually blocking; add test coverage and warn for common misconfiguration.

Do we:

  1. Bring that test change over to this issue to keep these separate
  2. or combine these issues?
bradjones1’s picture

Title: Update `symfony/http-foundation` to 4.4.44 » Update `symfony/http-foundation` to 4.4.44/6.1.3
andypost’s picture

Title: Update `symfony/http-foundation` to 4.4.44/6.1.3 » Require `symfony/http-foundation` 4.4.44/6.1.3 and later

Probably the upgrade should carry a fix as well because if someone will bump SF could get the error

bradjones1’s picture

Being bold and doing #9. We can rebase the PP'd issue once this is in.

bradjones1’s picture

Sorry for the noise on those last commits, I apparently forgot how to use git.

xjm’s picture

Patch updates area allowable in patch releases, but can we clarify what this is blocking? It's unclear from the IS and related issue.

xjm’s picture

Version: 10.0.x-dev » 9.5.x-dev
Issue tags: -Needs backport to 9.x
bradjones1’s picture

This blocks users of Apache+mod_php from using destructable services which run after the response is sent. The fix is partly upstream in Symfony (reason for this issue) and partly in the linked issue. Updating symfony/http-foundation unlocks the definitive fix.

xjm’s picture

Is this a regression? What introduced it?

I saw https://www.drupal.org/pift-ci-job/2439178 -- is that related?

bradjones1’s picture

Is this a regression? What introduced it?

Yes. But it's only the test that is incorrect/regressed.

The linked PP'd issue led me to discover that destructable services on Apache+mod_php were actually blocking. Except, there's no existing test coverage for this so it took my manual testing on a product locally to discover the underlying issue.

That led to raising https://github.com/symfony/symfony/pull/46931 which Fabien committed in 2 days (pretty proud of that!) which adds a flush() call to Symfony's request sending/output buffer flush routine.

As I outline in some detail at #3295790-11: Post-response task running (destructable services) are actually blocking; add test coverage and warn for common misconfiguration the change in symfony/http-foundation regresses the vary header test which you note here. Apache's mod_deflate will add a vary header during request flushing (that is, after PHP hands it over.) This is the correct behavior on Apache/PHP's part, it's Drupal's test coverage that didn't take this into account. (Because same issue as above - no existing test coverage. The test runners do use mod_php, though, which is why we get the test failure.)

All of this, except for the http-foundation upgrade, is in the MR on the linked issue. The vary header test that regresses here is related but orthogonal to the response-sending stuff, though, so it seems this is the smallest possible change we can make here relating to the version upgrade.

To put it another way - the version of Symfony released today means the constraints currently in drupal/core and drupal/core-recommended would allow you to install 4.4.44 on Drupal 9 and that causes this regression in the test suite. (I don't think it would actually break anything for sites, since it's just the test coverage that is incorrect.) The upstream PR was treated as a bugfix in Symfony.

Hope that helps.

andypost’s picture

I think in this issue we can just fix test to pass on 4.4.44+ and earlier to todo pointing to drupal.org/i/3295790

bradjones1’s picture

Why the @todo? Or are you talking about the backport to 9.3.x and 9.4.x would have that note? I don't think there's anything blocking the related issue getting in, once this is committed, so it's not really a @todo? The test is set up in such a way that it passes on zero or 1 vary headers, and only the one we expect... so the test will still pass on < 4.4.44.

xjm’s picture

Thanks @bradjones1.

The linked PP'd issue led me to discover that destructable services on Apache+mod_php were actually blocking.

But blocking what? That's what's unclear in the title, the IS, etc. The sentence is incomplete.

One more question -- is it necessary to increase the core constraint? We can definitely update the version in the lockfile and core-recommended, but the bug sounds pretty obscure otherwise, so I don't think we should necessarily force all sites to use 4.4.44 if they are not affected. Usually we only increase constraints if core is fundamentally broken or insecure on the old version.

This is especially relevant because we would want to backport this to 9.4.x and 9.3.x -- or at least the fix to the test, if it does indeed pass with or without the update -- so that those updated dependency test environments would continue to pass. However, we would not want to force sites to update to 4.4.44 on those versions because they could have a different conflict with that release. If sites are affected by the bug, they can already update to 4.4.44 themselves -- our constraints allow it.

bradjones1’s picture

Oh jeez, I understand why we are talking past one another - blocking in this case means, blocking the response. Without the fix on #3295790: Post-response task running (destructable services) are actually blocking; add test coverage and warn for common misconfiguration, services that are marked as destructable do not run after the response is sent, as is the intent. They instead run before the response is sent to the client, which could mean a major performance penalty because the entire point is to postpone this processing until after the response is sent to the client. This isn't always the case (e.g., if you're using FPM) but it should work on all major servers, I think.

One more question -- is it necessary to increase the core constraint? ... the bug sounds pretty obscure otherwise, so I don't think we should necessarily force all sites to use 4.4.44 if they are not affected. Usually we only increase constraints if core is fundamentally broken or insecure on the old version.

I will defer to others on this, but I can speak for myself that I was pretty surprised to find that destructable services didn't work at all as advertised. I know these are different issues, but fixing that issue (which is still marked as major) requires 4.4.44, so I think it's appropriate to do at least in 9.5.x. I don't necessarily expect the other issue to get backported unless it's "that" serious. I did think about it before I included the version constraint in the drupal/core composer.json but I think this is a case where site owners could definitely be thinking they are getting a performance boost that they are, in fact, not. And it's difficult enough to test manually.

Thanks for your attention and thoughtful consideration.

catch’s picture

I don't think we should increase the constraint here, just the version in composer.lock, this fixes a bug, but it's not a situation where a site is going to run into fatal errors or a security issue without the update.

bradjones1’s picture

Okie dokie. 9.5.x patch updated; 10.0.x can stay the same I suppose.

andypost’s picture

Category: Task » Bug report
Status: Needs review » Reviewed & tested by the community

I bet the CI-bot will be green!

andypost’s picture

Title: Require `symfony/http-foundation` 4.4.44/6.1.3 and later » Fix failed test on `symfony/http-foundation` 4.4.44/6.1.3 and later

I think this title is better

  • catch committed 7c1c60e on 10.0.x
    Issue #3300773 by bradjones1, andypost: Fix failed test on `symfony/http...
  • catch committed a5929f5 on 10.1.x
    Issue #3300773 by bradjones1, andypost: Fix failed test on `symfony/http...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 10.1.x, cherry-picked to 10.0.x and 9.5.x, thanks!

#3295790: Post-response task running (destructable services) are actually blocking; add test coverage and warn for common misconfiguration should probably only go into 9.5.x, so leaving things there on this issue too.

xjm’s picture

Version: 9.5.x-dev » 9.3.x-dev
Status: Fixed » Needs work

Can we get a version of this for 9.4.x and 9.3.x that only fixes the test, so that our "updated deps" test environments will pass on those branches?

  • catch committed 9d3e263 on 9.5.x
    Issue #3300773 by bradjones1, andypost: Fix failed test on `symfony/http...
catch’s picture

#27 is a good plan.

  • catch committed 92af65f on 9.5.x
    Revert "Issue #3300773 by bradjones1, andypost: Fix failed test on `...
catch’s picture

Reverted from 9.5.x due to https://www.drupal.org/pift-ci-job/2440361

Needs a 9.5.x MR/patch for the composer lock hash. there's already a 9.5.x MR here, might just need a re-roll, sent for re-test for now.

catch’s picture

Sent the 9.5.x MR for a retest.

bradjones1’s picture

Test-only MRs opened for 9.3.x and 9.4.x.

bradjones1’s picture

Status: Needs work » Needs review

  • catch committed 152704c on 9.5.x
    Issue #3300773 by bradjones1, andypost, xjm, catch: Fix failed test on `...
catch’s picture

Trying 9.5.x again, see what happens this time.

Committed 152704c and pushed to 9.5.x. Thanks!

  • xjm committed 7653952 on 9.5.x
    Issue #3300773 followup by xjm: Fix lockfile hash on 9.5.x.
    
xjm’s picture

9.5.x failed again, but I pushed a commit which should hopefully fix it. Looks like the lockfile was not updated when the constraint increase was removed and/or rebased on top of last week's release for Diactoros.

catch’s picture

This is really odd because several HEAD tests passed after the second commit too, I wonder if we're somehow getting a 'random pass' from that test sometimes.

xjm’s picture

Status: Needs review » Reviewed & tested by the community

9.4.x and 9.3.x MRs look good, thanks!

alexpott’s picture

Re the lock file hash test - that’s not random - it’s related to php version. On D9 for all env other than PHP 7.3 the lock file hash test is meaningless because we run composer update (thereby fixing the lock hash) to update PHPUnit.

bradjones1’s picture

Queued the 9.3 patch to run with "updated deps" per xjm's original intent here. I couldn't queue the 9.4 one b/c it says "not mergeable," but I have been seeing that more frequently lately and I'm not quite sure it's correct/what that really means.

Spokje’s picture

@bradjones1: GitLab seems overly(?) worried about non-mergeable MRs if they are a/some commits behind the main branch, most of the time clicking on the (then grey) MR!xxxx link triggers a re-think by GitLab about the MR state and some/most of the times it comes back green and mergeable after it had a little think (the AJAX-throbber disappears after the thinking is done, going back a page to the d.o. issue will then represent the current state).

xjm’s picture

Manually rebasing the 9.4.x MR should allow tests to run if DrupalCI won't let it currently.

bradjones1’s picture

Thanks... good to know about the "re-think" with this integration. Seems they both got running and are both passing.

xjm’s picture

Looks like those passed; thanks @bradjones1.

Anyone else want to +1 this so I can commit it? :)

Spokje’s picture

- Checked that both MR!2569 against 9.4.x and MR!2570 against 9.3.x contain _only_ the test changes in MR!2566 and not any of the composer related changes.
- Green tests all-around on "updated deps"

+1 RTBC

  • xjm committed 1c8f4ab on 9.4.x
    Issue #3300773 by bradjones1, xjm, catch, andypost, Spokje: Fix failed...

  • xjm committed 85e6389 on 9.3.x
    Issue #3300773 by bradjones1, xjm, catch, andypost, Spokje: Fix failed...
xjm’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 9.4.x and 9.3.x. Thanks @Spokje!

xjm’s picture

Status: Fixed » Closed (fixed)

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