Fix dangling timer when lazy connection closes with pending commands

clue · clue · commit 6859bf79defe · 2020-09-25T12:48:54.000+02:00
This is a somewhat hidden and rare race condition. The faulty idle timer
would be started when the underlying Redis connection closes while the
lazy connection still has pending commands, such as a blocking BLPOP
operation. The timer would trigger some timer after the connection has
been closed and would try to close the connection again, thus
referencing undefined variables and causing a hard error in this case.

The idle timer should not be started when the connection is already
closed.
diff --git a/src/LazyClient.php b/src/LazyClient.php
@@ -201,7 +201,7 @@ public function idle()
     {
         --$this->pending;
 
-        if ($this->pending < 1 && $this->idlePeriod >= 0 && !$this->subscribed && !$this->psubscribed) {
+        if ($this->pending < 1 && $this->idlePeriod >= 0 && !$this->subscribed && !$this->psubscribed && $this->promise !== null) {
             $idleTimer =& $this->idleTimer;
             $promise =& $this->promise;
             $idleTimer = $this->loop->addTimer($this->idlePeriod, function () use (&$idleTimer, &$promise) {
diff --git a/tests/LazyClientTest.php b/tests/LazyClientTest.php
@@ -573,6 +573,33 @@ public function testUnsubscribeAfterSubscribeWillResolveWhenUnderlyingClientReso
         $promise->then($this->expectCallableOnceWith(array('unsubscribe', 'foo', 0)));
     }
 
+    public function testBlpopWillRejectWhenUnderlyingClientClosesWhileWaitingForResponse()
+    {
+        $closeHandler = null;
+        $deferred = new Deferred();
+        $client = $this->getMockBuilder('Clue\React\Redis\Client')->getMock();
+        $client->expects($this->once())->method('__call')->with('blpop')->willReturn($deferred->promise());
+        $client->expects($this->any())->method('on')->withConsecutive(
+            array('close', $this->callback(function ($arg) use (&$closeHandler) {
+                $closeHandler = $arg;
+                return true;
+            }))
+        );
+
+        $this->factory->expects($this->once())->method('createClient')->willReturn(\React\Promise\resolve($client));
+
+        $this->loop->expects($this->never())->method('addTimer');
+
+        $promise = $this->client->blpop('list');
+
+        $this->assertTrue(is_callable($closeHandler));
+        $closeHandler();
+
+        $deferred->reject($e = new \RuntimeException());
+
+        $promise->then(null, $this->expectCallableOnceWith($e));
+    }
+
     public function createCallableMockWithOriginalConstructorDisabled($array)
     {
         if (method_exists('PHPUnit\Framework\MockObject\MockBuilder', 'addMethods')) {

Original file line number	Diff line number	Diff line change
`@@ -201,7 +201,7 @@ public function idle()`
`201`	`201`	`{`
`202`	`202`	`--$this->pending;`
`203`	`203`
`204`		`- if ($this->pending < 1 && $this->idlePeriod >= 0 && !$this->subscribed && !$this->psubscribed) {`
	`204`	`+ if ($this->pending < 1 && $this->idlePeriod >= 0 && !$this->subscribed && !$this->psubscribed && $this->promise !== null) {`
`205`	`205`	`$idleTimer =& $this->idleTimer;`
`206`	`206`	`$promise =& $this->promise;`
`207`	`207`	`$idleTimer = $this->loop->addTimer($this->idlePeriod, function () use (&$idleTimer, &$promise) {`