Skip to content

Commit 11bc00a

Browse files
abnegateclaude
andcommitted
feat(client): bound the connect dial and handshake separately
A steady-state operation may legitimately wait a long time for a large or slow response; establishing a connection may not. The pool salvages a connection whose operation just failed by calling connect() through reconnect(), and behind a proxy that accepts instantly while its backend is unreachable, the dial always "succeeds" — the first SCRAM reply is what stalls. That salvage attempt then cost a second full receive timeout on top of the failure it was recovering from: a 10s operation deadline produced a measured 21.8s outage per dead pooled connection. The new connectTimeout bounds both the TCP dial and the handshake receives, and only those: the flag arming the shorter deadline is scoped to connect() with a finally, so a slow first real query never inherits it and a failed handshake never leaves it armed on a reused client. Defaults to $timeout, so existing callers keep their behaviour. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 parent 78818fd commit 11bc00a

2 files changed

Lines changed: 137 additions & 11 deletions

File tree

src/Client.php

Lines changed: 63 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,28 @@ class Client
5858
*/
5959
private float $timeout = 30.0;
6060

61+
/**
62+
* Deadline for establishing a connection: the TCP dial plus the SCRAM
63+
* handshake that connect() runs before returning a usable client.
64+
*
65+
* Separate from {@see $timeout} because the two answer different
66+
* questions. A steady-state operation may legitimately wait a long time
67+
* for a large or slow response. Establishing a connection may not: the
68+
* pool calls connect() through reconnect() to salvage a connection whose
69+
* operation just failed, and behind a proxy that accepts instantly while
70+
* its backend is unreachable, that salvage attempt cost a second full
71+
* receive timeout on top of the failure — doubling every outage the pool
72+
* was trying to shorten. Defaults to $timeout so existing callers keep
73+
* their current behaviour.
74+
*/
75+
private ?float $connectTimeout = null;
76+
77+
/**
78+
* True while connect() is running its SCRAM exchange, so receive() applies
79+
* the connect deadline rather than the steady-state one.
80+
*/
81+
private bool $handshaking = false;
82+
6183
/**
6284
* Defines commands Mongo uses over wire protocol.
6385
*/
@@ -157,6 +179,11 @@ class Client
157179
* Set this when the user was created in a database other than admin (e.g. the
158180
* application database itself).
159181
* @param float $timeout Socket / receive idle timeout in seconds (default 30).
182+
* @param float|null $connectTimeout Deadline for the TCP dial plus SCRAM
183+
* handshake, in seconds. Defaults to $timeout. Set it lower than
184+
* $timeout when connecting through a proxy, so a reconnect against an
185+
* unreachable backend fails fast instead of spending a second full
186+
* receive timeout.
160187
* @throws \Exception
161188
*/
162189
public function __construct(
@@ -169,7 +196,8 @@ public function __construct(
169196
bool $tls = false,
170197
array $tlsOptions = [],
171198
?string $authSource = null,
172-
float $timeout = 30.0
199+
float $timeout = 30.0,
200+
?float $connectTimeout = null
173201
) {
174202
if (empty($database)) {
175203
throw new \InvalidArgumentException('Database name cannot be empty');
@@ -189,12 +217,16 @@ public function __construct(
189217
if ($timeout <= 0) {
190218
throw new \InvalidArgumentException('Timeout must be greater than 0');
191219
}
220+
if ($connectTimeout !== null && $connectTimeout <= 0) {
221+
throw new \InvalidArgumentException('Connect timeout must be greater than 0');
222+
}
192223

193224
$this->id = uniqid('utopia.mongo.client');
194225
$this->database = $database;
195226
$this->host = $host;
196227
$this->port = $port;
197228
$this->timeout = $timeout;
229+
$this->connectTimeout = $connectTimeout;
198230

199231
// Only use coroutines if explicitly requested and we're in a coroutine context
200232
if ($useCoroutine) {
@@ -259,20 +291,33 @@ public function connect(): self
259291
if ($this->port <= 0 || $this->port > 65535) {
260292
throw new Exception('MongoDB port must be between 1 and 65535');
261293
}
262-
if (!$this->client->connect($this->host, $this->port, $this->timeout)) {
294+
$connectTimeout = $this->connectTimeout ?? $this->timeout;
295+
if (!$this->client->connect($this->host, $this->port, $connectTimeout)) {
263296
$this->invalidate();
264297
throw new Exception("Failed to connect to MongoDB at {$this->host}:{$this->port}");
265298
}
266299

267300
$this->isConnected = true;
268301

269-
[$payload, $db] = $this->auth->start();
302+
// The SCRAM exchange below runs through receive(), so the handshake has
303+
// to carry the connect deadline too — behind a proxy that accepts
304+
// instantly, the dial is never what stalls, the first server reply is.
305+
$this->handshaking = true;
270306

271-
$res = $this->query($payload, $db);
307+
try {
308+
[$payload, $db] = $this->auth->start();
272309

273-
[$payload, $db] = $this->auth->continue($res);
310+
$res = $this->query($payload, $db);
274311

275-
$this->query($payload, $db);
312+
[$payload, $db] = $this->auth->continue($res);
313+
314+
$this->query($payload, $db);
315+
} finally {
316+
// Always, so a slow first real query never inherits the short
317+
// handshake bound — and a failed handshake never leaves it armed
318+
// on a client the caller may reuse.
319+
$this->handshaking = false;
320+
}
276321

277322
return $this;
278323
}
@@ -475,6 +520,16 @@ public function send(mixed $data): stdClass|array|int
475520
return $this->receive();
476521
}
477522

523+
/**
524+
* The deadline a single receive() may spend waiting for the peer.
525+
*/
526+
private function receiveTimeout(): float
527+
{
528+
return $this->handshaking
529+
? ($this->connectTimeout ?? $this->timeout)
530+
: $this->timeout;
531+
}
532+
478533
/**
479534
* Receive a message from connection.
480535
*
@@ -495,7 +550,7 @@ private function receive(): stdClass|array|int
495550
$chunks = [];
496551
$receivedLength = 0;
497552
$responseLength = null;
498-
$deadline = \microtime(true) + $this->timeout;
553+
$deadline = \microtime(true) + $this->receiveTimeout();
499554

500555
do {
501556
if (\microtime(true) >= $deadline) {
@@ -540,7 +595,7 @@ private function receive(): stdClass|array|int
540595

541596
// Activity: extend idle deadline so large multi-chunk responses
542597
// are not cut off by a fixed wall-clock budget from the first byte.
543-
$deadline = \microtime(true) + $this->timeout;
598+
$deadline = \microtime(true) + $this->receiveTimeout();
544599

545600
$chunkLen = \strlen($chunk);
546601
$receivedLength += $chunkLen;

tests/ClientTest.php

Lines changed: 74 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -183,6 +183,66 @@ public function testConnectPassesConfiguredTimeout(): void
183183
$this->assertSame([['mongo', 27017, 7.5, 0]], $transport->connects);
184184
}
185185

186+
public function testConnectDialsAndHandshakesUnderTheConnectDeadline(): void
187+
{
188+
$transport = new SyncTransportDouble();
189+
$transport->open = false;
190+
$transport->receives = [
191+
['result' => $this->frame(['ok' => 1.0]), 'error' => 0],
192+
['result' => $this->frame(['ok' => 1.0]), 'error' => 0],
193+
];
194+
$client = $this->client($transport, timeout: 7.5, connectTimeout: 1.5);
195+
$this->set($client, 'auth', new AuthenticationDouble());
196+
197+
$client->connect();
198+
199+
$this->assertSame(
200+
[['mongo', 27017, 1.5, 0]],
201+
$transport->connects,
202+
'The dial must use the connect deadline, not the steady-state receive timeout',
203+
);
204+
$this->assertFalse(
205+
$this->get($client, 'handshaking'),
206+
'A completed handshake must restore the steady-state receive deadline',
207+
);
208+
}
209+
210+
public function testHandshakeSilenceFailsAtTheConnectDeadline(): void
211+
{
212+
// Behind a proxy that accepts instantly while its backend is
213+
// unreachable, the dial is never what stalls — the first handshake
214+
// reply is. Without a separate connect deadline the handshake waited
215+
// out the full receive timeout, doubling every outage the pool's
216+
// recovery was trying to shorten.
217+
$transport = new SyncTransportDouble();
218+
$transport->open = false;
219+
$transport->receives = [
220+
['result' => '', 'error' => 0],
221+
['result' => '', 'error' => 0],
222+
['result' => '', 'error' => 0],
223+
];
224+
$client = $this->client($transport, timeout: 5.0, connectTimeout: 0.05);
225+
$this->set($client, 'auth', new AuthenticationDouble());
226+
227+
$startedAt = microtime(true);
228+
try {
229+
$client->connect();
230+
$this->fail('A silent handshake must fail at the connect deadline');
231+
} catch (Exception $exception) {
232+
$this->assertSame(11601, $exception->getCode());
233+
}
234+
235+
$this->assertLessThan(
236+
1.0,
237+
microtime(true) - $startedAt,
238+
'The silent handshake must fail at the connect deadline, not the receive timeout',
239+
);
240+
$this->assertFalse(
241+
$this->get($client, 'handshaking'),
242+
'A failed handshake must not leave the connect deadline armed on a reused client',
243+
);
244+
}
245+
186246
public function testSyncReceiveFailureHardClosesAndClearsState(): void
187247
{
188248
$transport = new SyncTransportDouble();
@@ -644,9 +704,20 @@ private function assertConnectionContextCleared(Client $client): void
644704
$this->assertNull($this->get($client, 'replicaSet'));
645705
}
646706

647-
private function client(SwooleClient|CoroutineClient $transport, float $timeout = 0.05): Client
648-
{
649-
$client = new Client('testing', 'mongo', 27017, 'root', 'example', timeout: $timeout);
707+
private function client(
708+
SwooleClient|CoroutineClient $transport,
709+
float $timeout = 0.05,
710+
?float $connectTimeout = null,
711+
): Client {
712+
$client = new Client(
713+
'testing',
714+
'mongo',
715+
27017,
716+
'root',
717+
'example',
718+
timeout: $timeout,
719+
connectTimeout: $connectTimeout,
720+
);
650721
$this->set($client, 'client', $transport);
651722

652723
return $client;

0 commit comments

Comments
 (0)