Skip to content

Commit cbabd21

Browse files
shubhamdhamaclaude
andcommitted
drpcwire,drpcstream: return the real error from a preempted send
A send blocked on backpressure used to return an opaque "interrupted" sentinel from WriteFrame, which the data path then remapped through CheckCancelError. WriteFrame lives in drpcwire and cannot know why a send was stopped, so it could only return a generic error and leave the stream to recover the cause. Pass the stream's send signal to WriteFrame directly and have it return that signal's error when the signal fires. The send signal is the write side's own "stop sending" signal, and terminate() always sets it, so a parked send is woken on every termination and returns send.Err(), which is io.EOF in the cancel and error cases. That is the same error the write loop's guard already returns when send is set before a frame is sent, so the parked and unparked paths now return the same thing. This is why the CheckCancelError remap on the write path is removed. It existed to turn the sentinel into the cancellation cause, but the write path now returns io.EOF, which is what gRPC reports on send; the real cause reaches the caller through the receive side, not the send side. Every cancel sets the send signal under the same lock, so the loop guard returns io.EOF before WriteFrame is even reached, and a send woken while parked returns it too. The only WriteFrame errors left are genuine transport failures, which happen only when the stream was not terminated, so there is no cancel cause to substitute and the raw error is correct. Passing a signal instead of a channel also lets WriteFrame resolve its channel lazily, only when a producer actually parks, so a stream that never hits backpressure no longer allocates that channel on every send. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 parent 5849e80 commit cbabd21

4 files changed

Lines changed: 37 additions & 22 deletions

File tree

drpcstream/stream.go

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -405,8 +405,13 @@ func (s *Stream) rawWriteLocked(kind drpcwire.Kind, data []byte) (err error) {
405405
drpcopts.GetStreamStats(&s.opts.Internal).AddWritten(uint64(len(fr.Data)))
406406
s.log("SEND", fr.String)
407407

408-
if err := s.wr.WriteFrame(fr, s.sigs.term.Signal()); err != nil {
409-
return s.CheckCancelError(errs.Wrap(err))
408+
// Pass the send signal, the write side's own stop signal, so a parked
409+
// WriteFrame is woken when sending ends (cancel, error, close) and
410+
// returns that signal's error directly. That is io.EOF in the cancel and
411+
// error cases, the same error the loop guard above returns, so the parked
412+
// and unparked paths agree and there is nothing to remap here.
413+
if err := s.wr.WriteFrame(fr, &s.sigs.send); err != nil {
414+
return err
410415
} else if fr.Done {
411416
return nil
412417
}

drpcstream/stream_test.go

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -487,7 +487,10 @@ func TestStream_SendCancelPreemptsBlockedSend(t *testing.T) {
487487
}
488488
select {
489489
case err := <-sendDone:
490-
assert.Error(t, err)
490+
// The preempted send reports the send signal's error (io.EOF), the same
491+
// as a send attempted after termination, per the gRPC convention. The
492+
// cancellation cause is available from the receive side.
493+
assert.Equal(t, err, io.EOF)
491494
case <-time.After(5 * time.Second):
492495
t.Fatal("blocked send was not interrupted by SendCancel")
493496
}

drpcwire/mux_writer.go

Lines changed: 18 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -7,18 +7,13 @@ import (
77
"io"
88
"sync"
99

10-
"github.com/zeebo/errs"
10+
"storj.io/drpc/drpcsignal"
1111
)
1212

1313
// defaultBufferCapacity is the initial capacity of the pending and in-flight
1414
// write buffers.
1515
var defaultBufferCapacity = 4096
1616

17-
// errInterrupted is returned by WriteFrame when its cancel channel fires while
18-
// it is blocked on backpressure. The caller maps it to the appropriate stream
19-
// error (e.g. via CheckCancelError).
20-
var errInterrupted = errs.New("sending frames interrupted")
21-
2217
// WriterOptions controls configuration settings for a MuxWriter.
2318
type WriterOptions struct {
2419
// MaximumBufferSize is the high-water mark for the pending write buffer.
@@ -135,11 +130,13 @@ func (mw *MuxWriter) run() {
135130

136131
// WriteFrame appends fr to the pending buffer. If the buffer is at its
137132
// high-water mark it blocks until run frees space, cancel fires, or the writer
138-
// is stopped. cancel is the caller's termination channel (e.g. a stream's term
139-
// signal); when it fires WriteFrame returns errInterrupted. A control-bit frame
140-
// is appended immediately even past the high-water mark, so an abortive cancel
141-
// is never delayed by backpressure.
142-
func (mw *MuxWriter) WriteFrame(fr Frame, cancel <-chan struct{}) (err error) {
133+
// is stopped. cancel is the caller's termination signal (e.g. a stream's send
134+
// signal); when it fires WriteFrame stops waiting and returns cancel.Err(), so
135+
// the caller gets the termination cause directly without interpreting a
136+
// sentinel. cancel may be nil to wait indefinitely for space. A control-bit
137+
// frame is appended immediately even past the high-water mark, so an abortive
138+
// cancel is never delayed by backpressure.
139+
func (mw *MuxWriter) WriteFrame(fr Frame, cancel *drpcsignal.Signal) (err error) {
143140
for {
144141
mw.mu.Lock()
145142
if mw.closed {
@@ -164,17 +161,25 @@ func (mw *MuxWriter) WriteFrame(fr Frame, cancel <-chan struct{}) (err error) {
164161
mw.blocked++
165162
mw.mu.Unlock()
166163

164+
// Resolve the cancel channel lazily, only now that we are parking, so the
165+
// common non-blocking path never forces the signal's channel to be
166+
// allocated.
167+
var cancelCh <-chan struct{}
168+
if cancel != nil {
169+
cancelCh = cancel.Signal()
170+
}
171+
167172
select {
168173
case <-ch:
169174
// Space may be available now; loop and re-check.
170175
mw.mu.Lock()
171176
mw.blocked--
172177
mw.mu.Unlock()
173-
case <-cancel:
178+
case <-cancelCh:
174179
mw.mu.Lock()
175180
mw.blocked--
176181
mw.mu.Unlock()
177-
return errInterrupted
182+
return cancel.Err()
178183
case <-mw.done:
179184
mw.mu.Lock()
180185
mw.blocked--

drpcwire/mux_writer_test.go

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,8 @@ import (
1212
"time"
1313

1414
"github.com/zeebo/assert"
15+
16+
"storj.io/drpc/drpcsignal"
1517
)
1618

1719
// blockingWriter blocks in Write until unblock is closed, then returns err.
@@ -399,22 +401,22 @@ func TestMuxWriter_WriteFrameBlocksUntilDrain(t *testing.T) {
399401
<-mw.Done()
400402
}
401403

402-
// A parked WriteFrame returns errInterrupted when its cancel channel fires.
404+
// A parked WriteFrame returns the cancel signal's error when it fires.
403405
func TestMuxWriter_WriteFrameCanceledWhileBlocked(t *testing.T) {
404406
bw := newBlockingWriter()
405407
mw := newTinyMuxWriter(bw)
406408
blockUntilFull(t, mw, bw)
407409

408-
cancel := make(chan struct{})
410+
var cancel drpcsignal.Signal
409411
done := make(chan error, 1)
410-
go func() { done <- mw.WriteFrame(RandFrame(), cancel) }()
412+
go func() { done <- mw.WriteFrame(RandFrame(), &cancel) }()
411413
assertBlocked(t, done)
412414

413-
close(cancel)
415+
cancelErr := errors.New("canceled")
416+
cancel.Set(cancelErr)
414417
select {
415418
case err := <-done:
416-
assert.Error(t, err)
417-
assert.Equal(t, err.Error(), errInterrupted.Error())
419+
assert.Equal(t, err, cancelErr)
418420
case <-time.After(5 * time.Second):
419421
t.Fatal("cancel did not unblock WriteFrame")
420422
}

0 commit comments

Comments
 (0)