8000 bug #32726 [Messenger] Fix redis last error not cleared between calls… · mkrauser/symfony@90c6482 · GitHub
[go: up one dir, main page]
Skip to content

Commit 90c6482

Browse files
fabpotfabpot
committed
bug symfony#32726 [Messenger] Fix redis last error not cleared between calls (chalasr)
This PR was merged into the 4.3 branch. Discussion ---------- [Messenger] Fix redis last error not cleared between calls | Q | A | ------------- | --- | Branch? | 4.3 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - Not clearing it gives misleading errors coming from previous calls which makes debugging hard. @alexander-schranz FYI Commits ------- 9c263ff [Messenger] Fix redis last error not cleared between calls
2 parents e2a2dd9 + 9c263ff commit 90c6482

File tree

2 files changed

+59
-15
lines changed

2 files changed

+59
-15
lines changed

‎src/Symfony/Component/Messenger/Tests/Transport/RedisExt/ConnectionTest.php

Lines changed: 29 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@
1616
use Symfony\Component\Messenger\Transport\RedisExt\Connection;
1717

1818
/**
19-
* @requires extension redis
19+
* @requires extension redis >= 4.3.0
2020
*/
2121
class ConnectionTest extends TestCase
2222
{
@@ -119,7 +119,7 @@ public function testUnexpectedRedisError()
119119
$redis->expects($this->once())->method('xreadgroup')->willReturn(false);
120120
$redis->expects($this->once())->method('getLastError')->willReturn('Redis error happens');
121121

122-
$connection = Connection::fromDsn('redis://localhost/queue', [], $redis);
122+
$connection = Connection::fromDsn('redis://localhost/queue', ['auto_setup' => false], $redis);
123123
$connection->get();
124124
}
125125

@@ -152,4 +152,31 @@ public function testGetNonBlocking()
152152
$connection->reject($message['id']);
153153
$redis->del('messenger-getnonblocking');
154154
}
155+
156+
public function testLastErrorGetsCleared()
157+
{
158+
$redis = $this->getMockBuilder(\Redis::class)->disableOriginalConstructor()->getMock();
159+
160+
$redis->expects($this->once())->method('xadd')->willReturn(0);
161+
$redis->expects($this->once())->method('xack')->willReturn(0);
162+
163+
$redis->method('getLastError')->willReturnOnConsecutiveCalls('xadd error', 'xack error');
164+
$redis->expects($this->exactly(2))->method('clearLastError');
165+
166+
$connection = Connection::fromDsn('redis://localhost/messenger-clearlasterror', ['auto_setup' => false], $redis);
167+
168+
try {
169+
$connection->add('message', []);
170+
} catch (TransportException $e) {
171+
}
172+
173+
$this->assertSame('xadd error', $e->getMessage());
174+
175+
try {
176+
$connection->ack('1');
177+
} catch (TransportException $e) {
178+
}
179+
180+
$this->assertSame('xack error', $e->getMessage());
181+
}
155182
}

‎src/Symfony/Component/Messenger/Transport/RedisExt/Connection.php

Lines changed: 30 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -114,12 +114,15 @@ public function get(): ?array
114114
1
115115
);
116116
} catch (\RedisException $e) {
117+
throw new TransportException($e->getMessage(), 0, $e);
117118
}
118119

119-
if ($e || false === $messages) {
120-
throw new TransportException(
121-
($e ? $e->getMessage() : $this->connection->getLastError()) ?? 'Could not read messages from the redis stream.'
122-
);
120+
if (false === $messages) {
121+
if ($error = $this->connection->getLastError() ?: null) {
122+
$this->connection->clearLastError();
123+
}
124+
125+
throw new TransportException($error ?? 'Could not read messages from the redis stream.');
123126
}
124127

125128
if ($this->couldHavePendingMessages && empty($messages[$this->stream])) {
@@ -144,28 +147,34 @@ public function get(): ?array
144147

145148
public function ack(string $id): void
146149
{
147-
$e = null;
148150
try {
149151
$acknowledged = $this->connection->xack($this->stream, $this->group, [$id]);
150152
} catch (\RedisException $e) {
153+
throw new TransportException($e->getMessage(), 0, $e);
151154
}
152155

153-
if ($e || !$acknowledged) {
154-
throw new TransportException(($e ? $e->getMessage() : $this->connection->getLastError()) ?? sprintf('Could not acknowledge redis message "%s".', $id), 0, $e);
156+
if (!$acknowledged) {
157+
if ($error = $this->connection->getLastError() ?: null) {
158+
$this->connection->clearLastError();
159+
}
160+
throw new TransportException($error ?? sprintf('Could not acknowledge redis message "%s".', $id));
155161
}
156162
}
157163

158164
public function reject(string $id): void
159165
{
160-
$e = null;
161166
try {
162167
$deleted = $this->connection->xack($this->stream, $this->group, [$id]);
163168
$deleted = $this->connection->xdel($this->stream, [$id]) && $deleted;
164169
} catch (\RedisException $e) {
170+
throw new TransportException($e->getMessage(), 0, $e);
165171
}
166172

167-
if ($e || !$deleted) {
168-
throw new TransportException(($e ? $e->getMessage() : $this->connection->getLastError()) ?? sprintf('Could not delete message "%s" from the redis stream.', $id), 0, $e);
173+
if (!$deleted) {
174+
if ($error = $this->connection->getLastError() ?: null) {
175+
$this->connection->clearLastError();
176+
}
177+
throw new TransportException($error ?? sprintf('Could not delete message "%s" from the redis stream.', $id));
169178
}
170179
}
171180

@@ -175,16 +184,19 @@ public function add(string $body, array $headers): void
175184
$this->setup();
176185
}
177186

178-
$e = null;
179187
try {
180188
$added = $this->connection->xadd($this->stream, '*', ['message' => json_encode(
181189
['body' => $body, 'headers' => $headers]
182190
)]);
183191
} catch (\RedisException $e) {
192+
throw new TransportException($e->getMessage(), 0, $e);
184193
}
185194

186-
if ($e || !$added) {
187-
throw new TransportException(($e ? $e->getMessage() : $this->connection->getLastError()) ?? 'Could not add a message to the redis stream.', 0, $e);
195+
if (!$added) {
196+
if ($error = $this->connection->getLastError() ?: null) {
197+
$this->connection->clearLastError();
198+
}
199+
throw new TransportException($error ?? 'Could not add a message to the redis stream.');
188200
}
189201
}
190202

@@ -196,6 +208,11 @@ public function setup(): void
196208
throw new TransportException($e->getMessage(), 0, $e);
197209
}
198210

211+
// group might already exist, ignore
212+
if ($this->connection->getLastError()) {
213+
$this->connection->clearLastError();
214+
}
215+
199216
$this->autoSetup = false;
200217
}
201218
}

0 commit comments

Comments
 (0)
0