Improved shutdown behaviour in connection handler and added tests to improve coverage

Author: AndrewCarterUK

src/ConnectionHandler/ConnectionHandler.php

@@ -27,7 +27,7 @@ class ConnectionHandler implements LoggerAwareInterface
     /**
      * @var bool
      */
-    protected $shuttingDown;
+    protected $shutdown;
 
     /**
      * @var KernelInterface
@@ -65,7 +65,7 @@ public function __construct(KernelInterface $kernel, ConnectionInterface $connec
     {
         $this->setLogger((null === $logger) ? new NullLogger() : $logger);
 
-        $this->shuttingDown = false;
+        $this->shutdown     = false;
         $this->kernel       = $kernel;
         $this->connection   = $connection;
         $this->requests     = [];
@@ -100,9 +100,7 @@ public function ready()
      */
     public function shutdown()
     {
-        foreach ($this->requests as &$request) {
-            $request['keepAlive'] = false;
-        }
+        $this->shutdown = true;
     }
 
     /**
@@ -220,16 +218,15 @@ protected function endRequest($requestId, $appStatus = 0, $protocolStatus = Daem
         $content = pack('NCx3', $appStatus, $protocolStatus);
         $this->writeRecord($requestId, DaemonInterface::FCGI_END_REQUEST, $content);
 
-        if (isset($this->requests[$requestId])) {
-            $keepAlive = $this->requests[$requestId]['keepAlive'];
+        $keepAlive = $this->requests[$requestId]['keepAlive'];
 
+        if (isset($this->requests[$requestId]['builder'])) {
             $this->requests[$requestId]['builder']->clean();
-            unset($this->requests[$requestId]);
+        }
 
-            if (!$keepAlive) {
-                $this->close();
-            }
-        } else {
+        unset($this->requests[$requestId]);
+
+        if (!$keepAlive) {
             $this->close();
         }
     }
@@ -283,25 +280,25 @@ protected function processBeginRequestRecord($requestId, $contentData)
             throw new ProtocolException('Unexpected FCGI_BEGIN_REQUEST record');
         }
 
-        if ($this->shuttingDown) {
-            $this->endRequest($requestId, 0, DaemonInterface::FCGI_OVERLOADED);
-            return;
-        }
-
         $contentFormat = 'nrole/Cflags/x5';
 
         $content = unpack($contentFormat, $contentData);
 
         $keepAlive = DaemonInterface::FCGI_KEEP_CONNECTION & $content['flags'];
 
+        $this->requests[$requestId] = ['keepAlive' => $keepAlive];
+
+        if ($this->shutdown) {
+            $this->endRequest($requestId, 0, DaemonInterface::FCGI_OVERLOADED);
+            return;
+        }
+
         if (DaemonInterface::FCGI_RESPONDER !== $content['role']) {
             $this->endRequest($requestId, 0, DaemonInterface::FCGI_UNKNOWN_ROLE);
-        } else {
-            $this->requests[$requestId] = [
-                'keepAlive' => $keepAlive,
-                'builder'   => new RequestBuilder(),
-            ];
+            return;
         }
+
+        $this->requests[$requestId]['builder'] = new RequestBuilder();
     }
 
     /**

test/ConnectionHandler/ConnectionHandlerTest.php

@@ -175,6 +175,85 @@ public function testClosedConnection()
         $this->assertTrue($context['connection']->isClosed());
     }
 
+    /**
+     * Test that the daemon doesn't accept new requests once it has been
+     * shutdown but still handles old ones
+     */
+    public function testShutdown()
+    {
+        $response = new Response($this->toStream('Hello World'), 200);
+
+        $kernelCalled = false;
+
+        // Create test environment
+        $context = $this->createTestingContext(function (ServerRequestInterface $request) use ($response, &$kernelCalled) {
+            $this->assertEquals(['FOO' => 'bar'], $request->getServerParams());
+
+            $kernelCalled = true;
+
+            return $response;
+        });
+
+        // Write half of the first request (id 0)
+        $context['clientWrapper']->writeBeginRequestRecord(0, DaemonInterface::FCGI_RESPONDER, DaemonInterface::FCGI_KEEP_CONNECTION);
+        $context['clientWrapper']->writeParamsRecord(0, 'foo', 'bar');
+        $context['clientWrapper']->writeParamsRecord(0);
+
+        // Process first half of the first request
+        $context['handler']->ready();
+
+        // Trigger the shutdown method
+        $context['handler']->shutdown();
+
+        // Try creating a new request (id 1)
+        $context['clientWrapper']->writeBeginRequestRecord(1, DaemonInterface::FCGI_RESPONDER, DaemonInterface::FCGI_KEEP_CONNECTION);
+
+        // Process the attempt at a second request after the shutdown
+        $context['handler']->ready();
+
+        // The application should end the second request immediately
+        $record = $context['clientWrapper']->readRecord($this);
+        $this->assertEquals(DaemonInterface::FCGI_END_REQUEST, $record['type']);
+        $this->assertEquals(1, $record['requestId']);
+
+        // The application should declare an overloaded protocol status
+        $content = unpack('NappStatus/CprotocolStatus/x3', $record['contentData']);
+        $this->assertEquals(DaemonInterface::FCGI_OVERLOADED, $content['protocolStatus']);
+
+        // Check daemon hasn't closed server side connection
+        $this->assertFalse($context['connection']->isClosed());
+
+        // Write the second half of the first request
+        $context['clientWrapper']->writeStdinRecord(0);
+
+        // Process the second half of the first request
+        $context['handler']->ready();
+
+        // Assert that kernel was called
+        $this->assertTrue($kernelCalled);
+
+        // Receive response
+        $rawResponse = '';
+
+        do {
+            $record = $context['clientWrapper']->readRecord($this);
+
+            $this->assertEquals(0, $record['requestId']);
+
+            if (DaemonInterface::FCGI_STDOUT === $record['type']) {
+                $rawResponse .= $record['contentData'];
+            }
+        } while (DaemonInterface::FCGI_END_REQUEST !== $record['type']);
+
+        $expectedResponse = "Status: 200 OK\r\n\r\nHello World";
+  
+        // Check response
+        $this->assertEquals($expectedResponse, $rawResponse);
+
+        // Close the client side socket
+        fclose($context['sockets'][0]);
+    }
+
     /**
      * Test that the daemon correctly handles a kernel exception.
      */
@@ -403,7 +482,7 @@ public function testUnknownRole()
 
         // The application should respond with an end request record
         $record = $context['clientWrapper']->readRecord($this);
-        $this->assertEquals(DaemonInterface::FCGI_END_REQUEST,  $record['type']);
+        $this->assertEquals(DaemonInterface::FCGI_END_REQUEST, $record['type']);
 
         // The application should declare an unknown role protocol status
         $content = unpack('NappStatus/CprotocolStatus/x3', $record['contentData']);