Issue #3086179 by alexpott, greg.1.anderson, Mixologic, heddn, Charlie ChX...

Issue #3086179 by alexpott, greg.1.anderson, Mixologic, heddn, Charlie ChX Negyesi: Fix spurious failures attempting to allocate a web server in the build tests

core/tests/Drupal/BuildTests/Framework/BuildTestBase.php

@@ -113,21 +113,22 @@ abstract class BuildTestBase extends TestCase {
   private $hostPort;
 
   /**
-   * The Mink session manager.
+   * A list of ports used by the test.
    *
-   * @var \Behat\Mink\Mink
-   */
-  private $mink;
-
-  /**
-   * @var \Symfony\Component\Lock\Factory
+   * 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 torn down.
+   *
+   * @var \Symfony\Component\Lock\LockInterface[]
    */
-  private $lockFactory;
+  private $portLocks = [];
 
   /**
-   * @var \Symfony\Component\Lock\Lock[]
+   * The Mink session manager.
+   *
+   * @var \Behat\Mink\Mink
    */
-  private $locks;
+  private $mink;
 
   /**
    * The most recent command process.
@@ -169,6 +170,9 @@ protected function tearDown() {
     parent::tearDown();
 
     $this->stopServer();
+    foreach ($this->portLocks as $lock) {
+      $lock->release();
+    }
     $ws = $this->getWorkspaceDirectory();
     $fs = new SymfonyFilesystem();
     if ($this->destroyBuild && $fs->exists($ws)) {
@@ -423,7 +427,7 @@ protected function instantiateServer($port, $working_dir = NULL) {
       }
       usleep(1000);
     }
-    throw new \RuntimeException('Unable to start the web server.');
+    throw new \RuntimeException(sprintf("Unable to start the web server.\nERROR OUTPUT:\n%s", $ps->getErrorOutput()));
   }
 
   /**
@@ -449,11 +453,29 @@ protected function stopServer() {
    *   Thrown when there are no available ports within the range.
    */
   protected function findAvailablePort() {
+    $store = new FlockStore(DrupalFilesystem::getOsTemporaryDirectory());
+    $lock_factory = new Factory($store);
+
     $counter = 100;
     while ($counter--) {
-      $port = random_int(1024, 65535);
-      if ($this->checkPortIsAvailable($port)) {
-        return $port;
+      // Limit to 9999 as higher ports cause random fails on DrupalCI.
+      $port = random_int(1024, 9999);
+
+      if (isset($this->portLocks[$port])) {
+        continue;
+      }
+
+      // Take a lock so that no other process can use the same port number even
+      // if the server is yet to start.
+      $lock = $lock_factory->createLock('drupal-build-test-port-' . $port);
+      if ($lock->acquire()) {
+        if ($this->checkPortIsAvailable($port)) {
+          $this->portLocks[$port] = $lock;
+          return $port;
+        }
+        else {
+          $lock->release();
+        }
       }
     }
     throw new \RuntimeException('Unable to find a port available to run the web server.');
@@ -468,18 +490,15 @@ protected function findAvailablePort() {
    * @return bool
    */
   protected function checkPortIsAvailable($port) {
-    if ($this->lockAcquired($port)) {
-      $fp = @fsockopen(self::$hostName, $port, $errno, $errstr, 1);
-      // If fsockopen() fails to connect, probably nothing is listening.
-      // It could be a firewall but that's impossible to detect, so as a
-      // best guess let's return it as available.
-      if ($fp === FALSE) {
-        return TRUE;
-      }
-      else {
-        $this->lockRelease($port);
-        fclose($fp);
-      }
+    $fp = @fsockopen(self::$hostName, $port, $errno, $errstr, 1);
+    // If fsockopen() fails to connect, probably nothing is listening.
+    // It could be a firewall but that's impossible to detect, so as a
+    // best guess let's return it as available.
+    if ($fp === FALSE) {
+      return TRUE;
+    }
+    else {
+      fclose($fp);
     }
     return FALSE;
   }
@@ -498,54 +517,6 @@ protected function getPortNumber() {
     return $this->hostPort;
   }
 
-  /**
-   * Get a lock.
-   *
-   * @param $name
-   *   The name of the lock.
-   *
-   * @return bool
-   *   TRUE if the lock has been successfully acquired.
-   */
-  protected function lockAcquired($name) {
-    if (!$this->lockFactory) {
-      $store = new FlockStore(DrupalFilesystem::getOsTemporaryDirectory());
-      $this->lockFactory = new Factory($store);
-    }
-    $name = $this->getLockName($name);
-    if (!isset($this->locks[$name])) {
-      $this->locks[$name] = $this->lockFactory->createLock($name);
-    }
-    $lock = $this->locks[$name];
-    return $lock->isAcquired() || $lock->acquire();
-  }
-
-  /**
-   * Releases a lock.
-   *
-   * @param $name
-   *   The name of the lock.
-   */
-  protected function lockRelease($name) {
-    $name = $this->getLockName($name);
-    if (isset($this->lock[$name])) {
-      $this->locks[$name]->release();
-    }
-  }
-
-  /**
-   * Gets the lock name.
-   *
-   * @param $name
-   *   The lock name.
-   *
-   * @return string
-   *   Prefix lock name.
-   */
-  protected function getLockName($name) {
-    return 'drupal-build-test-' . $name;
-  }
-
   /**
    * Copy the current working codebase into a workspace.
    *

core/tests/Drupal/BuildTests/Framework/Tests/BuildTestTest.php

@@ -129,6 +129,11 @@ public function testPortMany() {
       $this->assertTrue($processes[$port]->isRunning(), 'Process on port ' . $port . ' is not still running.');
       $this->assertFalse($this->checkPortIsAvailable($port));
     }
+
+    // Clean up after ourselves.
+    foreach ($processes as $process) {
+      $process->stop();
+    }
   }
 
 }