Fix spurious failures attempting to allocate a web server in the build tests

We could mark it incomplete to help out the 8.8.x effort.

if we *reduced* the time to something unreasonable, like 100ms, would / should we be able to predictably get this to fail, and thus have more confidence that the cure will help?

For cause #2 there shouldnt be much acquiring or releasing ports in the docker container. It might be that acquiring ports, from within a docker container, takes longer than we'd expect.

Maybe:

1. do_cli_server
2. php_cli_server_ctor
3. php_network_listen_socket
4. php_network_getaddresses
5. getaddrinfo

Are we well set up on DrupalCI for a getaddrinfo syscall? I mean, it shouldn't be a problem because it's hardwired to 127.0.0.1 but I have seen a lot worse than a misconfigured nsswitch.conf especially in the limited world of containers.

however, if there are any other processes running anywhere else on the same machine that are occasionally acquiring and releasing ports, then this could cause a race condition with our code.

Should we limit to registered ports (1024 to 49151) and leave the dynamic ports (49152 to 65535) alone? Simple enough change: $port = random_int(1024, 65535); is the current version.

Note it is always \Drupal\BuildTests\Framework\Tests\BuildTestTest::testPortMany.

So let's set it to repeat and also try making it 5 secs.

Whoops...

So I don't think our locking is working at people are expecting...

RuntimeException: Unable to start the web server.


ERROR_OUTPUT:
[Mon Oct  7 11:15:18 2019] Failed to listen on 127.0.0.1:59410 (reason: Address already in use)

So this test is almost designed to fail :)

\Drupal\BuildTests\Framework\BuildTestBase::lockAcquired() returns true if a lock is acquired or if the lock has already been acquired. Therefore a test like \Drupal\BuildTests\Framework\Tests\BuildTestTest::testPortMany() can get the same port because it has already acquired it. Also I'm not sure that we need to store all the locks on BuildTestBase.

\Drupal\BuildTests\Framework\BuildTestBase::checkPortIsAvailable() shouldn't be involved in locking... it needs to be moved to \Drupal\BuildTests\Framework\BuildTestBase::findAvailablePort() which is where locking is needed.

I don't think we need 5 seconds.

So we can't let the lock objects destruct until we're done with them... so let's store them in state. I think this all shows that providing generic locking methods on the test base class is not a great idea.

Maybe it's the timeout on the fsockopen... v weird.

So maybe it's because we don't stop the processes... also trying smaller port numbers because hey why not.

Yay... so was it the port range or the tidying up.

Looks like definitely the port range. I wonder if there's something about how docker, running on top of aws, handles dynamic ports, and maybe it takes longer for those to close.

But hey, restricting the range seems sensible to me. we only really need 32 of them (maybe 64 or 128 someday) at a time for parallelization, and even then we only need it for this test and once we have some future tests written to use this test type.

So this goes back to #29 and also provides a final patch.

I think all the changes this makes are worthwhile:

• Where we do the locking is moved to findAvailablePort from checkPortIsAvailable - this makes sense - we shouldn't be doing any locking in checkPortIsAvailable
• Removing the lock helper methods - locking is complex and I don't think we should have generic methods on the Test base class - it's not as if now the sub classes have a common need to lock.
• Cleaning up at the end of \Drupal\BuildTests\Framework\Tests\BuildTestTest::testPortMany()

+++ b/core/tests/Drupal/BuildTests/Framework/BuildTestBase.php
@@ -449,11 +439,33 @@ protected function stopServer() {
+      // Take a lock so that no other process can can the same port number even

Fun little nit. Made me smile: can use the... I don't think we are https://www.youtube.com/watch?v=T59EDTqqW0A

@heddn thanks for the review

+++ b/core/tests/Drupal/BuildTests/Framework/BuildTestBase.php
@@ -449,11 +439,33 @@ protected function stopServer() {
+      $port = random_int(1024, 9999);

We should comment why 9999 since this seems like a bit of a magic number.

Otherwise, I think these changes are fine.

+++ b/core/tests/Drupal/BuildTests/Framework/BuildTestBase.php
@@ -449,11 +439,33 @@ protected function stopServer() {
+    // Prevent the same process finding the same port by storing a list of ports
+    // already discovered. This also stores locks so they are not released until
+    // the test class is destroyed.
+    static $ports = [];

This shouldn't be static, because otherwise we never release lock objects until the runtime is over. It should be a class property that works the same way: $this->ports[$port_number] = $lock; It'll also need to be unset in tearDown().

Under run-tests.sh it works either way because we run one test class per process, so runtime ends pretty quickly. Under phpunit, however, those locks will be alive until after all tests have run, and all reports are generated. (There are about a thousand variations of this test listener which unsets test class properties for this reason. https://packagist.org/packages/mybuilder/phpunit-accelerator )

Also, since it's per object now and not per class, subsequent objects can start over at 8000 or whatever, and not be disallowed from trying a port that might have opened since the last poll.

@heddn & @Mile23 good calls...

I'm satisfied if this comes back green.

Committed 9a70cb8 and pushed to 8.8.x. Thanks!