diff --git a/src/Client.php b/src/Client.php index 25436df..1718719 100644 --- a/src/Client.php +++ b/src/Client.php @@ -58,6 +58,29 @@ class Client */ private float $timeout = 30.0; + /** + * Deadline for establishing a connection: the TCP dial plus the SCRAM + * handshake that connect() runs before returning a usable client. + * + * Separate from {@see $timeout} because the two answer different + * questions. A steady-state operation may legitimately wait a long time + * for a large or slow response. Establishing a connection may not: the + * pool calls connect() through reconnect() to salvage a connection whose + * operation just failed, and behind a proxy that accepts instantly while + * its backend is unreachable, that salvage attempt cost a second full + * receive timeout on top of the failure — doubling every outage the pool + * was trying to shorten. Defaults to $timeout so existing callers keep + * their current behaviour. + */ + private ?float $connectTimeout = null; + + /** + * Absolute deadline for the in-flight connect() — the dial and the SCRAM + * exchange share it, so however the budget is split between phases, the + * whole attempt is bounded once. Null outside connect(). + */ + private ?float $handshakeDeadline = null; + /** * Defines commands Mongo uses over wire protocol. */ @@ -157,6 +180,11 @@ class Client * Set this when the user was created in a database other than admin (e.g. the * application database itself). * @param float $timeout Socket / receive idle timeout in seconds (default 30). + * @param float|null $connectTimeout Deadline for the TCP dial plus SCRAM + * handshake, in seconds. Defaults to $timeout. Set it lower than + * $timeout when connecting through a proxy, so a reconnect against an + * unreachable backend fails fast instead of spending a second full + * receive timeout. * @throws \Exception */ public function __construct( @@ -169,7 +197,8 @@ public function __construct( bool $tls = false, array $tlsOptions = [], ?string $authSource = null, - float $timeout = 30.0 + float $timeout = 30.0, + ?float $connectTimeout = null ) { if (empty($database)) { throw new \InvalidArgumentException('Database name cannot be empty'); @@ -189,12 +218,16 @@ public function __construct( if ($timeout <= 0) { throw new \InvalidArgumentException('Timeout must be greater than 0'); } + if ($connectTimeout !== null && $connectTimeout <= 0) { + throw new \InvalidArgumentException('Connect timeout must be greater than 0'); + } $this->id = uniqid('utopia.mongo.client'); $this->database = $database; $this->host = $host; $this->port = $port; $this->timeout = $timeout; + $this->connectTimeout = $connectTimeout; // Only use coroutines if explicitly requested and we're in a coroutine context if ($useCoroutine) { @@ -259,20 +292,43 @@ public function connect(): self if ($this->port <= 0 || $this->port > 65535) { throw new Exception('MongoDB port must be between 1 and 65535'); } - if (!$this->client->connect($this->host, $this->port, $this->timeout)) { - $this->invalidate(); - throw new Exception("Failed to connect to MongoDB at {$this->host}:{$this->port}"); - } + // One absolute deadline for the whole attempt: the dial and every SCRAM + // receive consume from the same budget, so a phase that eats most of it + // leaves only the remainder — not a fresh allowance — for the next. + // Behind a proxy that accepts instantly, the dial is never what stalls, + // the first server reply is. + $budget = $this->connectTimeout ?? $this->timeout; + $this->handshakeDeadline = \microtime(true) + $budget; - $this->isConnected = true; + try { + if (!$this->client->connect($this->host, $this->port, $budget)) { + $this->invalidate(); + throw new Exception("Failed to connect to MongoDB at {$this->host}:{$this->port}"); + } - [$payload, $db] = $this->auth->start(); + $this->isConnected = true; - $res = $this->query($payload, $db); + try { + [$payload, $db] = $this->auth->start(); - [$payload, $db] = $this->auth->continue($res); + $res = $this->query($payload, $db); - $this->query($payload, $db); + [$payload, $db] = $this->auth->continue($res); + + $this->query($payload, $db); + } catch (\Throwable $error) { + // A dialed-but-unauthenticated socket must never be reused: the + // transport reports connected, so a later connect() would + // early-return without ever completing authentication. + $this->invalidate(); + throw $error; + } + } finally { + // Always, so a slow first real query never inherits the handshake + // bound — and a failed handshake never leaves it armed on a client + // the caller may reuse. + $this->handshakeDeadline = null; + } return $this; } @@ -475,6 +531,46 @@ public function send(mixed $data): stdClass|array|int return $this->receive(); } + /** + * The deadline a single receive() may spend waiting for the peer. + */ + private function receiveTimeout(): float + { + if ($this->handshakeDeadline === null) { + return $this->timeout; + } + + // The remainder of the connect budget, never a fresh allowance — so + // chunk-arrival renewals inside receive() shrink toward the absolute + // deadline instead of extending past it. + return \max(0.001, $this->handshakeDeadline - \microtime(true)); + } + + /** + * One socket-level wait, bounded by the caller's remaining budget. + * + * The deadline arithmetic in receive() means nothing if the blocking + * primitive itself waits on the constructor-time steady-state timeout: a + * peer that accepts and then goes silent parks the first recv() for that + * full window before any deadline is rechecked — which is exactly how a + * proxy fronting an unreachable backend behaves during connect(). + */ + private function recvWithin(float $seconds): string|false + { + if ($this->client instanceof CoroutineClient) { + return @$this->client->recv($seconds); + } + + // The synchronous client reads its per-operation timeout from the + // client options. Where a build applies options only at connect(), + // this degrades to the pre-existing behaviour — the socket waits the + // steady-state timeout — and the deadline check above still bounds + // the loop; where honoured, the socket-level wait matches the budget. + @$this->client->set(['timeout' => $seconds]); + + return @$this->client->recv(); + } + /** * Receive a message from connection. * @@ -495,15 +591,16 @@ private function receive(): stdClass|array|int $chunks = []; $receivedLength = 0; $responseLength = null; - $deadline = \microtime(true) + $this->timeout; + $deadline = \microtime(true) + $this->receiveTimeout(); do { - if (\microtime(true) >= $deadline) { + $remaining = $deadline - \microtime(true); + if ($remaining <= 0) { $this->invalidate(); throw new Exception('Receive timeout: no data received within reasonable time', 11601); } - $chunk = @$this->client->recv(); + $chunk = $this->recvWithin(\min($remaining, $this->timeout)); $errCode = $this->client->errCode ?? 0; // false => socket-level wait already elapsed with no payload. @@ -540,7 +637,7 @@ private function receive(): stdClass|array|int // Activity: extend idle deadline so large multi-chunk responses // are not cut off by a fixed wall-clock budget from the first byte. - $deadline = \microtime(true) + $this->timeout; + $deadline = \microtime(true) + $this->receiveTimeout(); $chunkLen = \strlen($chunk); $receivedLength += $chunkLen; diff --git a/tests/ClientTest.php b/tests/ClientTest.php index d43e8ff..ddb1712 100644 --- a/tests/ClientTest.php +++ b/tests/ClientTest.php @@ -33,9 +33,13 @@ private function connectTransport(string $host, int $port, float $timeout, int $ return $result; } - private function receiveTransport(): string|false + /** @var list */ + public array $receiveTimeouts = []; + + private function receiveTransport(float $timeout = 0.0): string|false { $this->events[] = ['receive']; + $this->receiveTimeouts[] = $timeout; $receive = array_shift($this->receives) ?? ['result' => '', 'error' => 0]; $this->errCode = $receive['error']; @@ -59,10 +63,22 @@ final class SyncTransportDouble extends SwooleClient public array $closes = []; + /** @var list */ + public array $timeoutOptions = []; + public function __construct() { } + public function set(array $settings): bool + { + if (isset($settings['timeout'])) { + $this->timeoutOptions[] = (float) $settings['timeout']; + } + + return true; + } + public function close(bool $force = false): bool { $this->events[] = ['close', $force]; @@ -124,7 +140,7 @@ public function isConnected(): bool public function recv(float $timeout = 0): string|false { - return $this->receiveTransport(); + return $this->receiveTransport($timeout); } public function send(string $data, float $timeout = 0): int|false @@ -183,6 +199,188 @@ public function testConnectPassesConfiguredTimeout(): void $this->assertSame([['mongo', 27017, 7.5, 0]], $transport->connects); } + public function testConnectDialsAndHandshakesUnderTheConnectDeadline(): void + { + $transport = new SyncTransportDouble(); + $transport->open = false; + $transport->receives = [ + ['result' => $this->frame(['ok' => 1.0]), 'error' => 0], + ['result' => $this->frame(['ok' => 1.0]), 'error' => 0], + ]; + $client = $this->client($transport, timeout: 7.5, connectTimeout: 1.5); + $this->set($client, 'auth', new AuthenticationDouble()); + + $client->connect(); + + $this->assertSame( + [['mongo', 27017, 1.5, 0]], + $transport->connects, + 'The dial must use the connect deadline, not the steady-state receive timeout', + ); + $this->assertNull( + $this->get($client, 'handshakeDeadline'), + 'A completed handshake must restore the steady-state receive deadline', + ); + } + + public function testHandshakeSilenceFailsAtTheConnectDeadline(): void + { + // Behind a proxy that accepts instantly while its backend is + // unreachable, the dial is never what stalls — the first handshake + // reply is. Without a separate connect deadline the handshake waited + // out the full receive timeout, doubling every outage the pool's + // recovery was trying to shorten. + $transport = new SyncTransportDouble(); + $transport->open = false; + $transport->receives = [ + ['result' => '', 'error' => 0], + ['result' => '', 'error' => 0], + ['result' => '', 'error' => 0], + ]; + $client = $this->client($transport, timeout: 5.0, connectTimeout: 0.05); + $this->set($client, 'auth', new AuthenticationDouble()); + + $startedAt = microtime(true); + try { + $client->connect(); + $this->fail('A silent handshake must fail at the connect deadline'); + } catch (Exception $exception) { + $this->assertSame(11601, $exception->getCode()); + } + + $this->assertLessThan( + 1.0, + microtime(true) - $startedAt, + 'The silent handshake must fail at the connect deadline, not the receive timeout', + ); + $this->assertNull( + $this->get($client, 'handshakeDeadline'), + 'A failed handshake must not leave the connect deadline armed on a reused client', + ); + } + + public function testDialAndHandshakeConsumeOneSharedConnectBudget(): void + { + // Each phase must receive the remainder of one absolute deadline — a + // fresh allowance per phase lets the complete attempt take a multiple + // of the configured bound (greptile P1 on #46). + $transport = new SyncTransportDouble(); + $client = $this->client($transport, timeout: 5.0, connectTimeout: 0.2); + $this->set($client, 'handshakeDeadline', microtime(true) + 0.2); + + $first = $this->invokeReceiveTimeout($client); + usleep(100_000); + $second = $this->invokeReceiveTimeout($client); + + $this->assertLessThan(0.21, $first); + $this->assertLessThan( + $first, + $second, + 'Handshake receives must consume the shared connect budget, not restart it', + ); + + $this->set($client, 'handshakeDeadline', null); + $this->assertSame(5.0, $this->invokeReceiveTimeout($client), 'Steady state must use the receive timeout'); + } + + public function testFailedHandshakeInvalidatesTheDialedSocket(): void + { + // A dialed-but-unauthenticated socket must never be reused: the + // transport reports connected, so a later connect() would early-return + // without ever completing authentication (CodeRabbit on #46). + $transport = new SyncTransportDouble(); + $transport->open = false; + $transport->receives = [ + ['result' => $this->frame(['ok' => 0.0, 'errmsg' => 'Authentication failed', 'code' => 18]), 'error' => 0], + ]; + $client = $this->client($transport, timeout: 0.2); + $this->set($client, 'auth', new AuthenticationDouble()); + + try { + $client->connect(); + $this->fail('A failed SCRAM exchange must throw'); + } catch (Exception) { + } + + $this->assertNotSame([], $transport->closes, 'The half-authenticated transport must be closed'); + $this->assertFalse($this->get($client, 'isConnected')); + + $transport->receives = [ + ['result' => $this->frame(['ok' => 1.0]), 'error' => 0], + ['result' => $this->frame(['ok' => 1.0]), 'error' => 0], + ]; + $transport->open = false; + $client->connect(); + + $this->assertCount( + 2, + $transport->connects, + 'A connect() after a failed handshake must dial again, never reuse the unauthenticated socket', + ); + } + + public function testSocketWaitsNeverExceedTheRemainingConnectBudget(): void + { + // The deadline arithmetic means nothing if the blocking primitive + // itself waits on the steady-state timeout: a peer that accepts and + // then goes silent parks the first recv() for that full window before + // any deadline is rechecked (greptile P1 x3 on #46 — the root). + $transport = new CoroutineTransportDouble(); + $transport->open = false; + $transport->receives = [ + ['result' => $this->frame(['ok' => 1.0]), 'error' => 0], + ['result' => $this->frame(['ok' => 1.0]), 'error' => 0], + ]; + $client = $this->client($transport, timeout: 5.0, connectTimeout: 0.2); + $this->set($client, 'auth', new AuthenticationDouble()); + + $client->connect(); + + $this->assertNotSame([], $transport->receiveTimeouts); + foreach ($transport->receiveTimeouts as $wait) { + $this->assertGreaterThan(0.0, $wait); + $this->assertLessThanOrEqual( + 0.2, + $wait, + 'A handshake socket wait must be bounded by the remaining connect budget, ' + . 'never the steady-state receive timeout', + ); + } + + $transport->receives = [['result' => $this->frame(['ok' => 1.0]), 'error' => 0]]; + $transport->receiveTimeouts = []; + $client->query(['ping' => 1]); + + $this->assertNotSame([], $transport->receiveTimeouts); + $this->assertEqualsWithDelta( + 5.0, + $transport->receiveTimeouts[0], + 0.05, + 'Steady-state socket waits must use the full receive timeout again', + ); + } + + public function testSyncSocketWaitsRefreshTheClientTimeoutOption(): void + { + // The synchronous client cannot take a per-call timeout; the budget + // travels through the client options instead, refreshed per wait. + $transport = new SyncTransportDouble(); + $transport->open = false; + $transport->receives = [ + ['result' => $this->frame(['ok' => 1.0]), 'error' => 0], + ['result' => $this->frame(['ok' => 1.0]), 'error' => 0], + ]; + $client = $this->client($transport, timeout: 5.0, connectTimeout: 0.2); + $this->set($client, 'auth', new AuthenticationDouble()); + + $client->connect(); + + $this->assertNotSame([], $transport->timeoutOptions); + foreach ($transport->timeoutOptions as $wait) { + $this->assertLessThanOrEqual(0.2, $wait); + } + } + public function testSyncReceiveFailureHardClosesAndClearsState(): void { $transport = new SyncTransportDouble(); @@ -644,9 +842,20 @@ private function assertConnectionContextCleared(Client $client): void $this->assertNull($this->get($client, 'replicaSet')); } - private function client(SwooleClient|CoroutineClient $transport, float $timeout = 0.05): Client - { - $client = new Client('testing', 'mongo', 27017, 'root', 'example', timeout: $timeout); + private function client( + SwooleClient|CoroutineClient $transport, + float $timeout = 0.05, + ?float $connectTimeout = null, + ): Client { + $client = new Client( + 'testing', + 'mongo', + 27017, + 'root', + 'example', + timeout: $timeout, + connectTimeout: $connectTimeout, + ); $this->set($client, 'client', $transport); return $client; @@ -675,6 +884,14 @@ private function receive(Client $client): mixed return $reflection->invoke($client); } + private function invokeReceiveTimeout(Client $client): float + { + $reflection = new ReflectionMethod(Client::class, 'receiveTimeout'); + $reflection->setAccessible(true); + + return $reflection->invoke($client); + } + private function receiveException(Client $client): Exception { try {