Skip to content

Commit ef33138

Browse files
abnegateclaude
andcommitted
fix(client): bound the socket-level wait itself, not just the loop
Third finding on the same seam, and the root of all three: the deadline arithmetic in receive() means nothing while the blocking primitive answers only to the constructor-time steady-state timeout. A peer that accepts and then goes silent — a proxy fronting an unreachable backend, the exact case connect() is being bounded for — parks the first recv() for the full steady-state window before any deadline is rechecked. Every socket wait now takes the remaining budget of the loop it serves: per-call on the coroutine client, via a per-wait refresh of the client timeout option on the synchronous one. Where a synchronous build applies options only at connect(), the behaviour degrades to exactly what shipped before — the loop-level deadline still bounds the total — and where honoured, the wait matches the budget. The contract tests observe the socket layer directly: every handshake wait stays within the remaining connect budget, steady-state waits return to the full receive timeout, and the synchronous transport sees its option refreshed per wait. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 parent c023ef1 commit ef33138

2 files changed

Lines changed: 108 additions & 4 deletions

File tree

src/Client.php

Lines changed: 28 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -546,6 +546,31 @@ private function receiveTimeout(): float
546546
return \max(0.001, $this->handshakeDeadline - \microtime(true));
547547
}
548548

549+
/**
550+
* One socket-level wait, bounded by the caller's remaining budget.
551+
*
552+
* The deadline arithmetic in receive() means nothing if the blocking
553+
* primitive itself waits on the constructor-time steady-state timeout: a
554+
* peer that accepts and then goes silent parks the first recv() for that
555+
* full window before any deadline is rechecked — which is exactly how a
556+
* proxy fronting an unreachable backend behaves during connect().
557+
*/
558+
private function recvWithin(float $seconds): string|false
559+
{
560+
if ($this->client instanceof CoroutineClient) {
561+
return @$this->client->recv($seconds);
562+
}
563+
564+
// The synchronous client reads its per-operation timeout from the
565+
// client options. Where a build applies options only at connect(),
566+
// this degrades to the pre-existing behaviour — the socket waits the
567+
// steady-state timeout — and the deadline check above still bounds
568+
// the loop; where honoured, the socket-level wait matches the budget.
569+
@$this->client->set(['timeout' => $seconds]);
570+
571+
return @$this->client->recv();
572+
}
573+
549574
/**
550575
* Receive a message from connection.
551576
*
@@ -569,12 +594,13 @@ private function receive(): stdClass|array|int
569594
$deadline = \microtime(true) + $this->receiveTimeout();
570595

571596
do {
572-
if (\microtime(true) >= $deadline) {
597+
$remaining = $deadline - \microtime(true);
598+
if ($remaining <= 0) {
573599
$this->invalidate();
574600
throw new Exception('Receive timeout: no data received within reasonable time', 11601);
575601
}
576602

577-
$chunk = @$this->client->recv();
603+
$chunk = $this->recvWithin(\min($remaining, $this->timeout));
578604
$errCode = $this->client->errCode ?? 0;
579605

580606
// false => socket-level wait already elapsed with no payload.

tests/ClientTest.php

Lines changed: 80 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -33,9 +33,13 @@ private function connectTransport(string $host, int $port, float $timeout, int $
3333
return $result;
3434
}
3535

36-
private function receiveTransport(): string|false
36+
/** @var list<float> */
37+
public array $receiveTimeouts = [];
38+
39+
private function receiveTransport(float $timeout = 0.0): string|false
3740
{
3841
$this->events[] = ['receive'];
42+
$this->receiveTimeouts[] = $timeout;
3943
$receive = array_shift($this->receives) ?? ['result' => '', 'error' => 0];
4044
$this->errCode = $receive['error'];
4145

@@ -59,10 +63,22 @@ final class SyncTransportDouble extends SwooleClient
5963

6064
public array $closes = [];
6165

66+
/** @var list<float> */
67+
public array $timeoutOptions = [];
68+
6269
public function __construct()
6370
{
6471
}
6572

73+
public function set(array $settings): bool
74+
{
75+
if (isset($settings['timeout'])) {
76+
$this->timeoutOptions[] = (float) $settings['timeout'];
77+
}
78+
79+
return true;
80+
}
81+
6682
public function close(bool $force = false): bool
6783
{
6884
$this->events[] = ['close', $force];
@@ -124,7 +140,7 @@ public function isConnected(): bool
124140

125141
public function recv(float $timeout = 0): string|false
126142
{
127-
return $this->receiveTransport();
143+
return $this->receiveTransport($timeout);
128144
}
129145

130146
public function send(string $data, float $timeout = 0): int|false
@@ -303,6 +319,68 @@ public function testFailedHandshakeInvalidatesTheDialedSocket(): void
303319
);
304320
}
305321

322+
public function testSocketWaitsNeverExceedTheRemainingConnectBudget(): void
323+
{
324+
// The deadline arithmetic means nothing if the blocking primitive
325+
// itself waits on the steady-state timeout: a peer that accepts and
326+
// then goes silent parks the first recv() for that full window before
327+
// any deadline is rechecked (greptile P1 x3 on #46 — the root).
328+
$transport = new CoroutineTransportDouble();
329+
$transport->open = false;
330+
$transport->receives = [
331+
['result' => $this->frame(['ok' => 1.0]), 'error' => 0],
332+
['result' => $this->frame(['ok' => 1.0]), 'error' => 0],
333+
];
334+
$client = $this->client($transport, timeout: 5.0, connectTimeout: 0.2);
335+
$this->set($client, 'auth', new AuthenticationDouble());
336+
337+
$client->connect();
338+
339+
$this->assertNotSame([], $transport->receiveTimeouts);
340+
foreach ($transport->receiveTimeouts as $wait) {
341+
$this->assertGreaterThan(0.0, $wait);
342+
$this->assertLessThanOrEqual(
343+
0.2,
344+
$wait,
345+
'A handshake socket wait must be bounded by the remaining connect budget, '
346+
. 'never the steady-state receive timeout',
347+
);
348+
}
349+
350+
$transport->receives = [['result' => $this->frame(['ok' => 1.0]), 'error' => 0]];
351+
$transport->receiveTimeouts = [];
352+
$client->query(['ping' => 1]);
353+
354+
$this->assertNotSame([], $transport->receiveTimeouts);
355+
$this->assertEqualsWithDelta(
356+
5.0,
357+
$transport->receiveTimeouts[0],
358+
0.05,
359+
'Steady-state socket waits must use the full receive timeout again',
360+
);
361+
}
362+
363+
public function testSyncSocketWaitsRefreshTheClientTimeoutOption(): void
364+
{
365+
// The synchronous client cannot take a per-call timeout; the budget
366+
// travels through the client options instead, refreshed per wait.
367+
$transport = new SyncTransportDouble();
368+
$transport->open = false;
369+
$transport->receives = [
370+
['result' => $this->frame(['ok' => 1.0]), 'error' => 0],
371+
['result' => $this->frame(['ok' => 1.0]), 'error' => 0],
372+
];
373+
$client = $this->client($transport, timeout: 5.0, connectTimeout: 0.2);
374+
$this->set($client, 'auth', new AuthenticationDouble());
375+
376+
$client->connect();
377+
378+
$this->assertNotSame([], $transport->timeoutOptions);
379+
foreach ($transport->timeoutOptions as $wait) {
380+
$this->assertLessThanOrEqual(0.2, $wait);
381+
}
382+
}
383+
306384
public function testSyncReceiveFailureHardClosesAndClearsState(): void
307385
{
308386
$transport = new SyncTransportDouble();

0 commit comments

Comments
 (0)