Problem/Motivation
Every day you get one more yard.
You take it on faith; you take it to the heart.
The waiting is the hardest part.
In #2917655: [9.4.x only] Drop official PHP 7.3 support in Drupal 9.4, we discovered that QuickStartTest
will wait forever for expected installer errors that never appear. The process timeout does not work. See https://dispatcher.drupalci.org/job/drupal_patches/115031/console for an example of the test timing out on the DrupalCI maximum execution time of 110 minutes (for a unit test that is normally done in less than a minute).
Proposed resolution
Use the correct APIs for waiting in a Symfony Process
.
The current merge request replaces the while
loops in QuickStartTest
with situationally appropriate APIs (via @longwave).
#22, #23, and #27 are failing testing patches to prove that the replacements for the while
loops now cause the tests to fail properly when the timeout is reached, rather than executing an infinite loop.
Remaining tasks
Need review.
User interface changes
N/A
API changes
N/A
Data model changes
N/A
Release notes snippet
N/A
Comment | File | Size | Author |
---|
Issue fork drupal-3265291
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:
Comments
Comment #3
xjmComment #4
xjmI checked and no other core tests that use
Symfony\Component\Process\Process
have such potentially infinite loops in them.Comment #5
xjmCritical really (blocks one, and makes the test un-debuggable).
Comment #6
longwaveShould we try to use
Process::wait()
or::waitUntil()
instead of handling this ourselves?Comment #7
xjmI was referring to the Process docs, which say:
I think
Process::wait()
is for when you have two asynchronously running processes, and you want one to wait on the other. Whereas this waiting within a single process, using a pattern in the docs.Comment #8
longwaveTo me the docs mean that you should run the loop yourself if you want to do any other processing while the process runs in the background, but you can use
wait()
if you just want to wait for it to finish.In the simple case we can just use the synchronous
::run()
.For the more complicated case we can use
::waitUntil()
, as implemented in the attached patch.I also think the 500 second timeout is really excessive; should we drop it to 30?
Comment #9
xjmHere's a test patch which should prove that the timeouts are now working as expected. Edit: Crosspost.
Comment #10
xjmThat seems risky; I wouldn't want to introduce a bunch of unpredictable random fails due to slow testbots. We should look back at the issue that added it originally and see if there's justification for the choice there.
Comment #11
xjmThe 500 timeout was added with the original introduction of the quick-start command.
Comment #12
xjmYeah, I read over the original issue and people were getting timeouts at 300 seconds, so they increased it to 500. This was probably with Composer 1, mind, so we might not need such a timeout anymore in D10. Still, I think it's best to keep it for now. 500 seconds is much better than the effectively infinite timeout currently. ;)
Comment #13
xjmThe fail-only patch in #9 fails as expected and proves that the timeouts are now working.
#8 fails
QuickStartTest
with:Comment #15
ressa CreditAttribution: ressa at Ardea commentedI am just following along here from the sideline, but loving the Tom Petty reference. And what a fine song, all the way from 1981: https://www.youtube.com/watch?v=uMyCa35_mOg
The frustration of the waiting can be excruciating. You start ... and wait ... and it breaks. Argh. That's also why I am so thankful for the gradual improvements in the Drupal code you all are working on here, as well as Composer with version 2.2. Every second (or even millisecond) counts.
Comment #16
xjm@ressa :)
I tested #8 locally and confirmed that it does make the processes respect the timeout; shortening the timeout to 1 second fails as expected. I also tested with the original change from #2917655: [9.4.x only] Drop official PHP 7.3 support in Drupal 9.4 on PHP 7.3 and confirmed that the test now times out correctly rather than getting stuck in an infinite loop. So that's probably a better approach; we just need to get that third hunk working.
Comment #18
xjmComment #19
xjmComment #21
xjmI moved #8 to a new MR and I think I fixed it.
Comment #22
xjmPatch of this combined with #2917655: [9.4.x only] Drop official PHP 7.3 support in Drupal 9.4 to demonstrate that PHP 7.3 should now time out properly rather than waiting forever.
Comment #23
xjmAnd, a version with artificially low timeouts to prove the other cases will time out properly as well.
Comment #25
xjm#22 has:
#23 has:
testQuickStartInstallAndServerCommands()
doesn't seem to be timing out, though.Comment #26
xjmAh, this will prove the timeout works for the last case -- it needs a failing condition.
Comment #27
xjmWith a dictionary word.
Comment #29
xjmThere we go:
This is ready for review again.
Comment #30
xjmUpdating the IS with a little more detail.
Comment #31
neclimdulAre we sure this isn't just hitting the race condition that was identified several years ago? #2975644: Random Failure in Drupal\Tests\Core\Command\QuickStartTest There's a pretty simple fix.
Comment #32
xjm@neclimdul, not in this case. It's definitely a 100% consistent
failnon-failing infinite loop whenever the conditions those three tests are looking for are not met (for example, if the behavior ofMINIMUM_SUPPORTED_PHP
is changed). See the console output linked in the IS. Edit: Or if it's a race condition, the race is with infinity. ;)It appears the tests were written with the assumption that built-in
Process
API for timeouts would terminate the test when needed despite the potentially infinite loops (programming 101...). However, Symfony's docs clearly indicate that the way the test was written before would have required manualcheckTimeout()
calls. I think @longwave's approach is cleaner though by using the actual API provided, which also works as proved in the test patches.Comment #33
xjmI think @longwave's approach will also fix #2975644: Random Failure in Drupal\Tests\Core\Command\QuickStartTest FWIW. Different bugs, one fix.
Comment #34
neclimdulIt doesn't. The first test I ran with these changes immediately failed. It looks like you can see the failure in the results on #8.
Comment #35
neclimdulWeird, I just ran it for an hour and it worked and I see where the fix similar to 2975644 is so probably good.
Comment #36
xjm@neclimdul, lol yep, I hacked those 500s timeouts down to 30s and then 1s locally so that it would complete sooner in the failing cases. Sounds like @longwave did too.
The fail in #8 was due to the loop exiting when only part of the output had been sent. I fixed that in 2f7382fab2.
Comment #38
xjmAdding credit for @Spokje, who was the one who figured out that this test was the reason #2917655: [9.4.x only] Drop official PHP 7.3 support in Drupal 9.4 was timing out.
Comment #39
neclimdulYeah, that race in #8 was what I documented in #21 in the other issue. 🤷 went ahead and closed it.
Comment #41
catchCommitted/pushed to 10.0.x, cherry-picked to 9.4.x and 9.3.x, thanks!