hashmail: block until stream is freed

Fix flaky tests. Reproducer:
go test -run TestHashMailServerReturnStream -count=20

TestHashMailServerReturnStream fails because the test cancels a read stream
and immediately dials RecvStream again expecting the same stream to be handed
out once the server returns it. The hashmail server implemented
RequestReadStream/RequestWriteStream with a non-blocking channel poll and
returned "read/write stream occupied" as soon as the mailbox was busy. That
raced with the deferred ReturnStream call and the reconnect often happened
before the stream got pushed back, so clients received the occupancy error
instead of the context cancellation they triggered.

Teach RequestReadStream/RequestWriteStream to wait for the stream to become
available (or the caller's context / server shutdown) with a bounded timeout.
If the wait expires we still return the "... stream occupied" error, so callers
that legitimately pile up can see that signal. The new streamAcquireTimeout
constant documents the policy, and the blocking select removes the race, so
reconnect attempts now either succeed or surface the original context error.
This commit is contained in:
Boris Nagaev
2025-11-26 20:09:36 -03:00
parent bf020ea103
commit e734b4a068
2 changed files with 26 additions and 3 deletions

View File

@@ -39,6 +39,12 @@ const (
// reads for it to be considered for pruning. Otherwise, memory will grow
// unbounded.
streamTTL = 24 * time.Hour
// streamAcquireTimeout determines how long we wait for a read/write
// stream to become available before reporting it as occupied. Context
// cancellation is still honoured immediately, so callers can shorten
// the wait.
streamAcquireTimeout = 250 * time.Millisecond
)
// streamID is the identifier of a stream.
@@ -317,7 +323,14 @@ func (s *stream) RequestReadStream(ctx context.Context) (*readStream, error) {
case r := <-s.readStreamChan:
s.status.streamTaken(true)
return r, nil
default:
case <-s.quit:
return nil, fmt.Errorf("stream shutting down")
case <-ctx.Done():
return nil, ctx.Err()
case <-time.After(streamAcquireTimeout):
return nil, fmt.Errorf("read stream occupied")
}
}
@@ -332,7 +345,14 @@ func (s *stream) RequestWriteStream(ctx context.Context) (*writeStream, error) {
case w := <-s.writeStreamChan:
s.status.streamTaken(false)
return w, nil
default:
case <-s.quit:
return nil, fmt.Errorf("stream shutting down")
case <-ctx.Done():
return nil, ctx.Err()
case <-time.After(streamAcquireTimeout):
return nil, fmt.Errorf("write stream occupied")
}
}

View File

@@ -512,7 +512,10 @@ func recvFromStream(client hashmailrpc.HashMailClient) error {
}()
select {
case <-time.After(time.Second):
// Wait a little longer than the server's stream-acquire timeout so we
// only trip this path when the server truly failed to hand over the
// stream (instead of beating it to the punch).
case <-time.After(2 * streamAcquireTimeout):
return fmt.Errorf("timed out waiting to receive from receive " +
"stream")