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.

Remove calls to sleep() added in 🐛 Post-response task running (destructable services) are actually blocking; add test coverage and warn for common misconfiguration Fixed .

More test groups (path,jsonapi,language,locale).

Looks like setWaitForTerminate() needs to run earlier in ConfigurableLanguageManagerTest and LocaleLocaleLookupTest.

Remove calls to sleep() added in 🐛 [random test failure] Random failure in PathWorkspacesTest Fixed .

Primary author of the changes in 🐛 Post-response task running (destructable services) are actually blocking; add test coverage and warn for common misconfiguration Fixed , 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.

We could try to use flock() for that purpose. If the state flag is set, the child site creates a temporary file and flocks() it (using LOCK_EX). The path to the file is communicated to the parent site in a response header (X-Drupal-Test-Wait-Terminate: /tmp/xxxx.lock).

The test runners HTTP client (most probably stuff in UIHelperTrait) examines the response headers and extracts the path from X-Drupal-Test-Wait-Terminate. If it exists, it tries to set an exclusive lock on that file (which blocks until the lock in the child site is released).

Some notes:

1. In the best case, the child site doesn't need to clean up the lock (since it should be released also by fclose(), or when stream is garbage collected.
2. The test runner assumes that the child already terminated if it fails to open the file or fails to lock it.

Opened an alternative MR 4461 implementing the ideas in #16 using the symfony lock component. Attached is a patch which executes the affected test groups 50 times in a row.

The last submitted patch, 18: 50x_functional_path_tests+MR-4461-17.patch, failed testing. View results →

Remove X-Drupal-Wait-Terminate response header and only attempt to wait for termination if container has state service.

Retain a reference to the lock, otherwise it will be released prematurely.

+++ 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.

This is outdated now.

Yes. And since we are not accessing the Request/Response object anymore it doesn't need to go into a stack middleware. Not sure if there is a better location for the code though.

The last submitted patch, 21: 50x_functional_path_tests+MR-4461-21.patch, failed testing. View results →

Switching to \Drupal:lock(). We know that this is working in other cases, so let's just use that.

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.

znerol → closed merge request !4434

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!

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

znerol → closed merge request !4461