Consistently emit close event after quit even if server rejects

clue · clue · commit 1e14223a35dd · 2023-11-30T10:01:50.000+01:00
diff --git a/README.md b/README.md
@@ -348,6 +348,11 @@ $mysql->query('CREATE TABLE test ...');
 $mysql->quit();
 ```
 
+This method will gracefully close the connection to the MySQL database
+server once all outstanding commands are completed. See also
+[`close()`](#close) if you want to force-close the connection without
+waiting for any commands to complete instead.
+
 #### close()
 
 The `close(): void` method can be used to
diff --git a/src/Io/Connection.php b/src/Io/Connection.php
@@ -135,17 +135,16 @@ public function ping()
     public function quit()
     {
         return new Promise(function ($resolve, $reject) {
-            $this->_doCommand(new QuitCommand())
-                ->on('error', function ($reason) use ($reject) {
-                    $reject($reason);
-                })
-                ->on('success', function () use ($resolve) {
-                    $this->state = self::STATE_CLOSED;
-                    $this->emit('end', [$this]);
-                    $this->emit('close', [$this]);
-                    $resolve(null);
-                });
+            $command = $this->_doCommand(new QuitCommand());
             $this->state = self::STATE_CLOSING;
+            $command->on('success', function () use ($resolve) {
+                $resolve(null);
+                $this->close();
+            });
+            $command->on('error', function ($reason) use ($reject) {
+                $reject($reason);
+                $this->close();
+            });
         });
     }
 
diff --git a/src/MysqlClient.php b/src/MysqlClient.php
@@ -7,8 +7,9 @@
 use React\EventLoop\LoopInterface;
 use React\Mysql\Io\Connection;
 use React\Mysql\Io\Factory;
-use React\Stream\ReadableStreamInterface;
+use React\Promise\Promise;
 use React\Socket\ConnectorInterface;
+use React\Stream\ReadableStreamInterface;
 
 /**
  * This class represents a connection that is responsible for communicating
@@ -135,9 +136,8 @@ function () {
                             // successfully disconnected => remove reference
                             $this->disconnecting = null;
                         },
-                        function () use ($connection) {
-                            // soft-close failed => force-close connection
-                            $connection->close();
+                        function () {
+                            // soft-close failed but will close anyway => remove reference
                             $this->disconnecting = null;
                         }
                     );
@@ -361,6 +361,11 @@ function (\Exception $e) {
      * $mysql->quit();
      * ```
      *
+     * This method will gracefully close the connection to the MySQL database
+     * server once all outstanding commands are completed. See also
+     * [`close()`](#close) if you want to force-close the connection without
+     * waiting for any commands to complete instead.
+     *
      * @return PromiseInterface<void>
      *     Resolves with a `void` value on success or rejects with an `Exception` on error.
      */
@@ -376,17 +381,25 @@ public function quit()
             return \React\Promise\resolve(null);
         }
 
-        return $this->connecting()->then(function (Connection $connection) {
-            $this->awake();
-            return $connection->quit()->then(
-                function () {
-                    $this->close();
-                },
-                function (\Exception $e) {
-                    $this->close();
-                    throw $e;
-                }
-            );
+        return new Promise(function (callable $resolve, callable $reject) {
+            $this->connecting()->then(function (Connection $connection) use ($resolve, $reject) {
+                $this->awake();
+                // soft-close connection and emit close event afterwards both on success or on error
+                $connection->quit()->then(
+                    function () use ($resolve){
+                        $resolve(null);
+                        $this->close();
+                    },
+                    function (\Exception $e) use ($reject) {
+                        $reject($e);
+                        $this->close();
+                    }
+                );
+            }, function (\Exception $e) use ($reject) {
+                // emit close event afterwards when no connection can be established
+                $reject($e);
+                $this->close();
+            });
         });
     }
 
diff --git a/tests/Io/ConnectionTest.php b/tests/Io/ConnectionTest.php
@@ -17,6 +17,72 @@ public function testQuitWillEnqueueOneCommand()
         $conn->quit();
     }
 
+    public function testQuitWillResolveBeforeEmittingCloseEventWhenQuitCommandEmitsSuccess()
+    {
+        $stream = $this->getMockBuilder('React\Socket\ConnectionInterface')->getMock();
+
+        $pingCommand = null;
+        $executor = $this->getMockBuilder('React\MySQL\Io\Executor')->setMethods(['enqueue'])->getMock();
+        $executor->expects($this->once())->method('enqueue')->willReturnCallback(function ($command) use (&$pingCommand) {
+            return $pingCommand = $command;
+        });
+
+        $connection = new Connection($stream, $executor);
+
+        $events = '';
+        $connection->on('close', function () use (&$events) {
+            $events .= 'closed.';
+        });
+
+        $this->assertEquals('', $events);
+
+        $promise = $connection->quit();
+
+        $promise->then(function () use (&$events) {
+            $events .= 'fulfilled.';
+        });
+
+        $this->assertEquals('', $events);
+
+        $this->assertNotNull($pingCommand);
+        $pingCommand->emit('success');
+
+        $this->assertEquals('fulfilled.closed.', $events);
+    }
+
+    public function testQuitWillRejectBeforeEmittingCloseEventWhenQuitCommandEmitsError()
+    {
+        $stream = $this->getMockBuilder('React\Socket\ConnectionInterface')->getMock();
+
+        $pingCommand = null;
+        $executor = $this->getMockBuilder('React\MySQL\Io\Executor')->setMethods(['enqueue'])->getMock();
+        $executor->expects($this->once())->method('enqueue')->willReturnCallback(function ($command) use (&$pingCommand) {
+            return $pingCommand = $command;
+        });
+
+        $connection = new Connection($stream, $executor);
+
+        $events = '';
+        $connection->on('close', function () use (&$events) {
+            $events .= 'closed.';
+        });
+
+        $this->assertEquals('', $events);
+
+        $promise = $connection->quit();
+
+        $promise->then(null, function () use (&$events) {
+            $events .= 'rejected.';
+        });
+
+        $this->assertEquals('', $events);
+
+        $this->assertNotNull($pingCommand);
+        $pingCommand->emit('error', [new \RuntimeException()]);
+
+        $this->assertEquals('rejected.closed.', $events);
+    }
+
     public function testQueryAfterQuitRejectsImmediately()
     {
         $stream = $this->getMockBuilder('React\Socket\ConnectionInterface')->getMock();
diff --git a/tests/MysqlClientTest.php b/tests/MysqlClientTest.php
@@ -1,6 +1,6 @@
 <?php
 
-namespace React\Tests\Mysql\Io;
+namespace React\Tests\Mysql;
 
 use React\Mysql\Io\Connection;
 use React\Mysql\MysqlClient;
@@ -10,7 +10,6 @@
 use React\Promise\PromiseInterface;
 use React\Stream\ReadableStreamInterface;
 use React\Stream\ThroughStream;
-use React\Tests\Mysql\BaseTestCase;
 
 class MysqlClientTest extends BaseTestCase
 {
@@ -197,12 +196,12 @@ public function testPingFollowedByIdleTimerWillQuitUnderlyingConnection()
         $timeout();
     }
 
-    public function testPingFollowedByIdleTimerWillCloseUnderlyingConnectionWhenQuitFails()
+    public function testPingFollowedByIdleTimerWillNotHaveToCloseUnderlyingConnectionWhenQuitFailsBecauseUnderlyingConnectionEmitsCloseAutomatically()
     {
         $base = $this->getMockBuilder('React\Mysql\Io\Connection')->setMethods(['ping', 'quit', 'close'])->disableOriginalConstructor()->getMock();
         $base->expects($this->once())->method('ping')->willReturn(\React\Promise\resolve(null));
         $base->expects($this->once())->method('quit')->willReturn(\React\Promise\reject(new \RuntimeException()));
-        $base->expects($this->once())->method('close');
+        $base->expects($this->never())->method('close');
 
         $factory = $this->getMockBuilder('React\Mysql\Io\Factory')->disableOriginalConstructor()->getMock();
         $factory->expects($this->once())->method('createConnection')->willReturn(\React\Promise\resolve($base));
@@ -227,6 +226,15 @@ public function testPingFollowedByIdleTimerWillCloseUnderlyingConnectionWhenQuit
 
         $this->assertNotNull($timeout);
         $timeout();
+
+        assert($base instanceof Connection);
+        $base->emit('close');
+
+        $ref = new \ReflectionProperty($connection, 'connecting');
+        $ref->setAccessible(true);
+        $connecting = $ref->getValue($connection);
+
+        $this->assertNull($connecting);
     }
 
     public function testPingAfterIdleTimerWillCloseUnderlyingConnectionBeforeCreatingSecondConnection()
@@ -757,6 +765,32 @@ public function testQuitAfterPingReturnsPendingPromiseWhenConnectionIsPending()
         $ret->then($this->expectCallableNever(), $this->expectCallableNever());
     }
 
+    public function testQuitAfterPingRejectsAndThenEmitsCloseWhenFactoryFailsToCreateUnderlyingConnection()
+    {
+        $deferred = new Deferred();
+        $factory = $this->getMockBuilder('React\MySQL\Io\Factory')->disableOriginalConstructor()->getMock();
+        $factory->expects($this->once())->method('createConnection')->willReturn($deferred->promise());
+        $loop = $this->getMockBuilder('React\EventLoop\LoopInterface')->getMock();
+
+        $connection = new MysqlClient('', null, $loop);
+
+        $ref = new \ReflectionProperty($connection, 'factory');
+        $ref->setAccessible(true);
+        $ref->setValue($connection, $factory);
+
+        $connection->ping()->then(null, $this->expectCallableOnce());
+
+        $this->expectOutputString('reject.close.');
+        $connection->on('close', function () {
+            echo 'close.';
+        });
+        $connection->quit()->then(null, function () {
+            echo 'reject.';
+        });
+
+        $deferred->reject(new \RuntimeException());
+    }
+
     public function testQuitAfterPingWillQuitUnderlyingConnectionWhenResolved()
     {
         $base = $this->getMockBuilder('React\Mysql\Io\Connection')->disableOriginalConstructor()->getMock();
@@ -777,11 +811,12 @@ public function testQuitAfterPingWillQuitUnderlyingConnectionWhenResolved()
         $connection->quit();
     }
 
-    public function testQuitAfterPingResolvesAndEmitsCloseWhenUnderlyingConnectionQuits()
+    public function testQuitAfterPingResolvesAndThenEmitsCloseWhenUnderlyingConnectionQuits()
     {
         $base = $this->getMockBuilder('React\Mysql\Io\Connection')->disableOriginalConstructor()->getMock();
+        $deferred = new Deferred();
         $base->expects($this->once())->method('ping')->willReturn(\React\Promise\resolve(null));
-        $base->expects($this->once())->method('quit')->willReturn(\React\Promise\resolve(null));
+        $base->expects($this->once())->method('quit')->willReturn($deferred->promise());
 
         $factory = $this->getMockBuilder('React\Mysql\Io\Factory')->disableOriginalConstructor()->getMock();
         $factory->expects($this->once())->method('createConnection')->willReturn(\React\Promise\resolve($base));
@@ -793,21 +828,25 @@ public function testQuitAfterPingResolvesAndEmitsCloseWhenUnderlyingConnectionQu
         $ref->setAccessible(true);
         $ref->setValue($connection, $factory);
 
-        $connection->on('close', $this->expectCallableOnce());
-
         $connection->ping();
-        $ret = $connection->quit();
 
-        $this->assertTrue($ret instanceof PromiseInterface);
-        $ret->then($this->expectCallableOnce(), $this->expectCallableNever());
+        $this->expectOutputString('quit.close.');
+        $connection->on('close', function () {
+            echo 'close.';
+        });
+        $connection->quit()->then(function () {
+            echo 'quit.';
+        });
+
+        $deferred->resolve(null);
     }
 
-    public function testQuitAfterPingRejectsAndEmitsCloseWhenUnderlyingConnectionFailsToQuit()
+    public function testQuitAfterPingRejectsAndThenEmitsCloseWhenUnderlyingConnectionFailsToQuit()
     {
-        $error = new \RuntimeException();
+        $deferred = new Deferred();
         $base = $this->getMockBuilder('React\Mysql\Io\Connection')->disableOriginalConstructor()->getMock();
         $base->expects($this->once())->method('ping')->willReturn(\React\Promise\resolve(null));
-        $base->expects($this->once())->method('quit')->willReturn(\React\Promise\reject($error));
+        $base->expects($this->once())->method('quit')->willReturn($deferred->promise());
 
         $factory = $this->getMockBuilder('React\Mysql\Io\Factory')->disableOriginalConstructor()->getMock();
         $factory->expects($this->once())->method('createConnection')->willReturn(\React\Promise\resolve($base));
@@ -819,13 +858,17 @@ public function testQuitAfterPingRejectsAndEmitsCloseWhenUnderlyingConnectionFai
         $ref->setAccessible(true);
         $ref->setValue($connection, $factory);
 
-        $connection->on('close', $this->expectCallableOnce());
-
         $connection->ping();
-        $ret = $connection->quit();
 
-        $this->assertTrue($ret instanceof PromiseInterface);
-        $ret->then($this->expectCallableNever(), $this->expectCallableOnceWith($error));
+        $this->expectOutputString('reject.close.');
+        $connection->on('close', function () {
+            echo 'close.';
+        });
+        $connection->quit()->then(null, function () {
+            echo 'reject.';
+        });
+
+        $deferred->reject(new \RuntimeException());
     }
 
     public function testCloseEmitsCloseImmediatelyWhenConnectionIsNotAlreadyPending()
diff --git a/tests/NoResultQueryTest.php b/tests/NoResultQueryTest.php
@@ -133,14 +133,35 @@ public function testPingWithValidAuthWillRunUntilQuitAfterPing()
         Loop::run();
     }
 
-    /**
-     * @doesNotPerformAssertions
-     */
+    public function testPingAndQuitWillFulfillPingBeforeQuitBeforeCloseEvent()
+    {
+        $this->expectOutputString('ping.quit.close.');
+
+        $uri = $this->getConnectionString();
+        $connection = new MysqlClient($uri);
+
+        $connection->on('close', function () {
+            echo 'close.';
+        });
+
+        $connection->ping()->then(function () {
+            echo 'ping.';
+        });
+
+        $connection->quit()->then(function () {
+            echo 'quit.';
+        });
+
+        Loop::run();
+    }
+
     public function testPingWithValidAuthWillRunUntilIdleTimerAfterPingEvenWithoutQuit()
     {
         $uri = $this->getConnectionString();
         $connection = new MysqlClient($uri);
 
+        $connection->on('close', $this->expectCallableNever());
+
         $connection->ping();
 
         Loop::run();
