Add a way to delay executions in test runner until terminate event completed in the child site

I've started to see test failures in my experimental database driver DruDbal, due to database locks, since when the parent issue was committed.

Bumping to major, even though I think critical would be appropriate if the testing framework stability decreased.

Primary author of the changes in #3295790: Post-response task running (destructable services) are actually blocking; add test coverage and warn for common misconfiguration, here.

From a quick read of this issue it appears that the goal is to disable post-response processing, by way of essentially undoing the fixes from the other issue, namely setting the content-length header. This might work, however it seems a bit hacky to me and, most importantly, not entirely obvious. For someone who comes along later and wants to maintain or debug this code, it's not clear why the content-length header is related. Even I had to go back and do some issue archaeology (that issue was open for almost a year, so my memory was a bit fuzzy and this stuff is pretty esoteric.)

For my part I would rather see a more explicit short-circuit of this functionality, or (better yet) improve the underlying race conditions and flakiness while allowing the underlying functionality work as expected, so we are actually testing the system as it's expected to operate. There was a lot of discussion in the parent issue about this kind of thing, and it was generally considered to be an improvement in test coverage because we're no longer testing/depending on broken functionality.

I don't love the additions of the sleep() calls, but we had little choice because as you rightly point out it's hard to signal to and from the test runner and the site that's being called. I think this is where the improvement should happen - I'm sure there is a more elegant solution to this aspect of this puzzle.

From a quick read of this issue it appears that the goal is to disable post-response processing, by way of essentially undoing the fixes from the other issue, namely setting the content-length header. This might work, however it seems a bit hacky to me and, most importantly, not entirely obvious.

This is true, but I think it applies equally to sleep(1).

For my part I would rather see a more explicit short-circuit of this functionality, or (better yet) improve the underlying race conditions and flakiness while allowing the underlying functionality work as expected, so we are actually testing the system as it's expected to operate.

One possible approach that crossed my mind was a test-only post response task, that writes something to state or key/value when it's finished and tries to run last of the post response tasks. We could then poll for that in the parent process, and delete it again as soon as its found ready for the next time, like a ::waitForResponseTasksToFinish() method. This would allow the post response tasks to run actually post-response while having something hopefully more reliable than sleep() to ensure they have.

+++ b/core/lib/Drupal/Core/CoreServiceProvider.php
@@ -142,6 +143,12 @@ protected function registerTest(ContainerBuilder $container) {
       ->addTag('http_client_middleware');
+    // Removes Content-Length header added in FinishResponseSubscriber if
+    // required by the test runner.
+    $container

This is outdated now. New approach looks very encouraging to me.

Just using lock seems both simpler and more reliable. Haven't done a line by line review yet but also nothing stuck out either, so +1.

I did a once-over of the revised patch and I'm a bit confused as to where and how it's actually waiting for termination of the post-response work. From my read of the earlier comments I figured that there would be, say, a service added that would run after all other destructable services that would release the lock, but it appears this is being done within the test HTTP client? Isn't the whole problem here that the client side doesn't know how long the post-response work takes, and has to be polling some state that is asynchronous from the request->response cycle? I could definitely be reading this wrong as I am not an expert on locks (I always get turned around when implementing them) but I'm suspicious that this might not be signaling the end of work so much as adding a few ticks, and as a result the timing works out?

@bradjones I think we need a comment as a reminder, but it's fine:

DatabaseLockBackend for example does this:

  /**
   * Constructs a new DatabaseLockBackend.
   *
   * @param \Drupal\Core\Database\Connection $database
   *   The database connection.
   */
  public function __construct(Connection $database) {
    // __destruct() is causing problems with garbage collections, register a
    // shutdown function instead.
    drupal_register_shutdown_function([$this, 'releaseAll']);
    $this->database = $database;
  }

So acquiring a lock at the beginning of the request, this will then run at the end of the request after all post response tasks have run, clearing out every lock that was acquired.

It's mostly a safety catch so that a request can't hold a lock even after it's finished (say if there's a fatal error before the lock can be released) but happens to do exactly what we need here.

The patch is using the default values for ::acquire() and ::wait() which means it's acquired for 30 seconds. Lock:wait() intelligently handles polling while it's waiting (i.e. it starts with millseconds and ends up at hundreds of milliseconds), so that works for making sure we wait the minimum time necessary.

Ahhh, yeah, OK. That makes sense and appreciate the clarification. I think a comment that states how it works is helpful here for maintainability. Thanks.

This seems ready for core comitters review, and is IMHO a much more deterministic way then whacking sleep(1) into tests.

Committed 1debb39 and pushed to 11.x. Thanks!