Problem/Motivation

standUpServer is called by visit, and even though we have a really nice comment:
// If there's not a server at this point, make one.
that would make you think we only start a new server conditionally, we seem to always start a new server. On every single visit.

This leaves dangling file locks and eats up server processes because we leave lots of hanging php servers that aren't ever stopped.

Proposed resolution

Do something like:

    // If there's not a server at this point, make one.
    if (!$this->serverProcess || $this->serverProcess->isTerminated()) {
      $this->serverProcess = $this->instantiateServer($this->getPortNumber(), $working_dir);
      if ($this->serverProcess) {
        $this->serverDocroot = $working_dir;
      }
    }

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

heddn created an issue. See original summary.

heddn’s picture

Issue summary: View changes
Status: Active » Needs review
FileSize
901 bytes
alexpott’s picture

+++ b/core/tests/Drupal/BuildTests/Framework/BuildTestBase.php
@@ -382,9 +382,11 @@ protected function standUpServer($working_dir = NULL) {
     // If there's not a server at this point, make one.
-    $this->serverProcess = $this->instantiateServer($this->getPortNumber(), $working_dir);
-    if ($this->serverProcess) {
-      $this->serverDocroot = $working_dir;
+    if (!$this->serverProcess || $this->serverProcess->isTerminated()) {
+      $this->serverProcess = $this->instantiateServer($this->getPortNumber(), $working_dir);
+      if ($this->serverProcess) {
+        $this->serverDocroot = $working_dir;
+      }
     }

Is this testable? Also it looks like the comment above needs an update.

heddn’s picture

It isn't really testable. Not sure the comment needs update, unless I'm missing something. Rather the new code is fulfilling the promise of the comment now.

Mile23’s picture

Adds a test.

heddn’s picture

I can confirm the failure test works:

$ vendor/bin/phpunit -c core core/tests/Drupal/BuildTests/Framework/Tests/BuildTestTest.php
PHPUnit 6.5.14 by Sebastian Bergmann and contributors.

Testing Drupal\BuildTests\Framework\Tests\BuildTestTest
....F                                                               5 / 5 (100%)

Time: 6.46 seconds, Memory: 6.00MB

There was 1 failure:

1) Drupal\BuildTests\Framework\Tests\BuildTestTest::testStandUpServer
Failed asserting that two variables reference the same object.

/var/www/html/core/tests/Drupal/BuildTests/Framework/Tests/BuildTestTest.php:153

FAILURES!
Tests: 5, Assertions: 77, Failures: 1.

Mile23’s picture

+++ b/core/tests/Drupal/BuildTests/Framework/Tests/BuildTestTest.php
@@ -136,4 +136,32 @@ public function testPortMany() {
+    $this->assertNotSame($first_process, $ref_process->getValue($this));
+    $second_process = $ref_process->getValue($this);

This always seemed awkward, so I fixed it.

Also, here's a test-only patch as well as the full one.

The last submitted patch, 7: 3088447_7_test_only.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

heddn’s picture

I wish I could RTBC this. I wrote the code, Paul wrote the test. I'm good w/ the test. If he's good with the code under test, can we mark this RTBC jointly ;)

Krzysztof Domański’s picture

Looks realy good! RTBC+1

Minor CS: expected 1 blank line after function.

--- tests/Drupal/BuildTests/Framework/Tests/BuildTestTest.php
+++ PHP_CodeSniffer
@@ -166,4 +166,5 @@
     $this->assertNotSame($first_process, $ref_process->getValue($this));
     $this->assertNotSame($second_process, $ref_process->getValue($this));
   }
+
 }
Mile23’s picture

Addressed CS issue.

Krzysztof Domański’s picture

Status: Needs review » Reviewed & tested by the community

Thanks!

alexpott’s picture

Status: Reviewed & tested by the community » Patch (to be ported)

Committed and pushed eecc0efc4e to 9.0.x and b02aad0305 to 8.9.x. Thanks!

  • alexpott committed eecc0ef on 9.0.x
    Issue #3088447 by Mile23, heddn, Krzysztof Domański: BuildTestBase->...

  • alexpott committed b02aad0 on 8.9.x
    Issue #3088447 by Mile23, heddn, Krzysztof Domański: BuildTestBase->...
alexpott’s picture

Status: Patch (to be ported) » Fixed

Discussed with @catch we agreed this was good to backport to 8.8.x

  • alexpott committed 58faaf5 on 8.8.x
    Issue #3088447 by Mile23, heddn, Krzysztof Domański: BuildTestBase->...

Status: Fixed » Closed (fixed)

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