Browse Source

Porperly handle -ERR instead of +QUEUED inside MULTI ... EXEC.

Error responses such as -OOM or -ERR on invalid arguments in commands
are returned immediatly instead of +QUEUED when using a transaction,
which is a condition that had not been tested enough. This condition
led to a bug in which Predis was not invalidating the transaction, so
when trying to create a new transaction Redis returned a "-ERR MULTI
calls can not be nested".

This commit fixes #187.
Daniele Alessandri 10 years ago
parent
commit
959734cf8c
2 changed files with 52 additions and 5 deletions
  1. 8 5
      lib/Predis/Transaction/MultiExec.php
  2. 44 0
      tests/Predis/Transaction/MultiExecTest.php

+ 8 - 5
lib/Predis/Transaction/MultiExec.php

@@ -190,18 +190,21 @@ class MultiExec implements BasicClientInterface, ExecutableContextInterface
     public function executeCommand(CommandInterface $command)
     {
         $this->initialize();
-        $response = $this->client->executeCommand($command);
 
         if ($this->state->isCAS()) {
-            return $response;
+            return $this->client->executeCommand($command);
         }
 
-        if ($response != 'QUEUED' && !$response instanceof StatusResponse) {
+        $response = $this->client->getConnection()->executeCommand($command);
+
+        if ($response instanceof StatusResponse && $response == 'QUEUED') {
+            $this->commands->enqueue($command);
+        } else if ($response instanceof ErrorResponseInterface) {
+            throw new AbortedMultiExecException($this, $response->getMessage());
+        } else {
             $this->onProtocolError('The server did not return a +QUEUED status response.');
         }
 
-        $this->commands->enqueue($command);
-
         return $this;
     }
 

+ 44 - 0
tests/Predis/Transaction/MultiExecTest.php

@@ -482,6 +482,50 @@ class MultiExecTest extends PredisTestCase
         $this->assertSame(array('MULTI', 'SET', 'ECHO', 'DISCARD'), self::commandsToIDs($commands));
     }
 
+    /**
+     * @group disconnected
+     */
+    public function testProperlyDiscardsTransactionAfterServerExceptionInBlock()
+    {
+        $connection = $this->getMockedConnection(function (CommandInterface $command) {
+            switch ($command->getId()) {
+                case 'MULTI':
+                    return true;
+
+                case 'ECHO':
+                    return new Response\Error('ERR simulated failure on ECHO');
+
+                case 'EXEC':
+                    return new Response\Error('EXECABORT Transaction discarded because of previous errors.');
+
+                default:
+                    return new Response\Status('QUEUED');
+            }
+        });
+
+        $client = new Client($connection);
+
+        // First attempt
+        $tx = new MultiExec($client);
+
+        try {
+            $tx->multi()->set('foo', 'bar')->echo('simulated failure')->exec();
+        } catch (\Exception $exception) {
+            $this->assertInstanceOf('Predis\Transaction\AbortedMultiExecException', $exception);
+            $this->assertSame('ERR simulated failure on ECHO', $exception->getMessage());
+        }
+
+        // Second attempt
+        $tx = new MultiExec($client);
+
+        try {
+            $tx->multi()->set('foo', 'bar')->echo('simulated failure')->exec();
+        } catch (\Exception $exception) {
+            $this->assertInstanceOf('Predis\Transaction\AbortedMultiExecException', $exception);
+            $this->assertSame('ERR simulated failure on ECHO', $exception->getMessage());
+        }
+    }
+
     /**
      * @group disconnected
      */