Skip to content

drpcstream: gate data-frame sends on the per-stream send window#71

Open
suj-krishnan wants to merge 1 commit into
mainfrom
sujatha/flow-control-send-gate
Open

drpcstream: gate data-frame sends on the per-stream send window#71
suj-krishnan wants to merge 1 commit into
mainfrom
sujatha/flow-control-send-gate

Conversation

@suj-krishnan

@suj-krishnan suj-krishnan commented Jul 1, 2026

Copy link
Copy Markdown

Wires the sendWindow credit gate into the stream write path:

  • In rawWriteLocked, a KindMessage frame acquires len(frame) bytes of per-stream send credit before it is handed to the writer.
  • Control kinds (invoke/metadata/etc.) bypass the gate — flow control applies to data frames only.
  • terminate closes the window with the send-side error (sigs.send.Err() — first-wins, so io.EOF when a cancel/error path pre-set it): a send parked on credit returns the same error as one parked in MuxWriter.WriteFrame or a later send. SendError's pre-close uses io.EOF and Close's uses termClosed for the same parked/unparked agreement.

@suj-krishnan
suj-krishnan force-pushed the sujatha/flow-control-send-window branch from 50c697f to a4ad9e8 Compare July 13, 2026 07:00
@suj-krishnan
suj-krishnan force-pushed the sujatha/flow-control-send-gate branch from 58a5fed to 30ffb52 Compare July 13, 2026 07:00
@suj-krishnan
suj-krishnan force-pushed the sujatha/flow-control-send-window branch from a4ad9e8 to 6ff7baf Compare July 13, 2026 07:06
@suj-krishnan
suj-krishnan force-pushed the sujatha/flow-control-send-gate branch from 30ffb52 to 9b6d87b Compare July 13, 2026 07:06
@suj-krishnan
suj-krishnan force-pushed the sujatha/flow-control-send-window branch from 6ff7baf to 7fb0331 Compare July 13, 2026 08:41
@suj-krishnan
suj-krishnan force-pushed the sujatha/flow-control-send-gate branch from 9b6d87b to 2512021 Compare July 13, 2026 08:41
@suj-krishnan
suj-krishnan force-pushed the sujatha/flow-control-send-window branch from 7fb0331 to 9c3bc23 Compare July 13, 2026 08:47
@suj-krishnan
suj-krishnan force-pushed the sujatha/flow-control-send-gate branch from 2512021 to c086e23 Compare July 13, 2026 08:47
@suj-krishnan
suj-krishnan force-pushed the sujatha/flow-control-send-window branch from 9c3bc23 to efc77b4 Compare July 13, 2026 08:55
@suj-krishnan
suj-krishnan force-pushed the sujatha/flow-control-send-gate branch from c086e23 to b14cb1f Compare July 13, 2026 08:55
@suj-krishnan
suj-krishnan force-pushed the sujatha/flow-control-send-window branch from efc77b4 to d88576b Compare July 13, 2026 08:59
@suj-krishnan
suj-krishnan force-pushed the sujatha/flow-control-send-gate branch from b14cb1f to 6594c14 Compare July 13, 2026 08:59
@suj-krishnan
suj-krishnan force-pushed the sujatha/flow-control-send-window branch 7 times, most recently from b1f5883 to 4d08d0b Compare July 13, 2026 12:29
@suj-krishnan
suj-krishnan force-pushed the sujatha/flow-control-send-gate branch 4 times, most recently from fe9d197 to 228866a Compare July 14, 2026 08:46
@suj-krishnan
suj-krishnan marked this pull request as ready for review July 14, 2026 09:51
@suj-krishnan suj-krishnan self-assigned this Jul 14, 2026
@suj-krishnan
suj-krishnan force-pushed the sujatha/flow-control-send-gate branch from 228866a to de7b9dc Compare July 14, 2026 10:00
@suj-krishnan
suj-krishnan force-pushed the sujatha/flow-control-send-gate branch 2 times, most recently from fcfa2e4 to 10b863f Compare July 14, 2026 10:25
@suj-krishnan
suj-krishnan force-pushed the sujatha/flow-control-send-window branch from 4d08d0b to deefd7b Compare July 15, 2026 11:03
@suj-krishnan
suj-krishnan force-pushed the sujatha/flow-control-send-gate branch 2 times, most recently from 7d03098 to 336c61c Compare July 16, 2026 09:08
@suj-krishnan
suj-krishnan force-pushed the sujatha/flow-control-send-window branch from deefd7b to af4331b Compare July 16, 2026 09:08
Base automatically changed from sujatha/flow-control-send-window to main July 16, 2026 09:10
@suj-krishnan
suj-krishnan force-pushed the sujatha/flow-control-send-gate branch 2 times, most recently from ef342ce to 0d5cf31 Compare July 17, 2026 05:38

@shubhamdhama shubhamdhama left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are the comments for the first commit.

Comment thread drpcstream/stream.go Outdated
defer s.checkFinished()

// Wait for the write lock without holding s.mu (see Close).
// Wait for the write lock without holding s.mu, matching CloseSend's

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment is related to the other PR.

Comment thread drpcstream/stream.go Outdated
Comment on lines +550 to +562
if s.sendw != nil {
s.sendw.close(io.EOF)
}
s.mu.Unlock()

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't need to be wrapped in s.mu locks. Both are concurrency safe.

Comment thread drpcstream/stream.go Outdated
Comment on lines +623 to +635
@@ -588,12 +624,29 @@
if s.sigs.term.IsSet() {
s.mu.Unlock()
return nil
}

// Close the send window before taking the write lock so a send parked on
// credit wakes and releases the lock, instead of stalling the close on a
// grant that may never come (mirrors SendCancel's signal-first ordering).
if s.sendw != nil {
s.sendw.close(termClosed)
}
s.mu.Unlock()

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here: This doesn't need to be wrapped in s.mu locks. Both are concurrency safe.

Comment thread drpcstream/stream.go Outdated
s.mu.Unlock()
return nil
}
s.mu.Unlock()

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one too: This doesn't need to be wrapped in s.mu locks. This is concurrency safe.

Comment thread drpcstream/stream.go
Comment on lines 624 to 627
if s.sigs.term.IsSet() {
s.mu.Unlock()
return nil
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also from a correctness point of view this isn't required, but we can keep it as a "fast-path". So not asking for any change here.

Comment thread drpcstream/stream.go Outdated
// disabled) leaves sends ungated. acquire parks until credit arrives,
// the ctx is canceled, or the window closes (stream termination).
if kind == drpcwire.KindMessage && s.sendw != nil {
if err := s.sendw.acquire(s.Context(), int64(len(fr.Data))); err != nil {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s.Context() is useless here. close does the trick of aborting. I would recommend just keep the n and remove the context.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch - the PR is updated.

Comment thread drpcstream/stream.go Outdated
if s.sigs.send.IsSet() || s.sigs.term.IsSet() {
s.mu.Unlock()
return nil
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Close, SendError, and CloseSend now all have nearly identical lock unlocks, maybe we should add in a small helper that could reduce the duplication?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have removed the changes from this PR for now as they are slightly adjacent to flow control - we'll tackle it separately

@suj-krishnan
suj-krishnan force-pushed the sujatha/flow-control-send-gate branch from 1520087 to 6649f34 Compare July 21, 2026 14:24
Wire the sendWindow credit gate into rawWriteLocked: a KindMessage frame
acquires len(frame) bytes of per-stream send credit before it is handed
to the writer. Control frames (invoke/metadata) bypass the gate.

The window is opt-in: a stream has no send window by default, so data
writes stay ungated (unlimited) and behavior is unchanged until one is
installed. terminate closes the window with the send-side error
(sigs.send is first-wins, holding io.EOF when a cancel/error path
pre-set it), so a send parked on credit returns the same error as one
parked in WriteFrame or a later send. SendCancel and Cancel already
terminate before taking the write lock, so they wake a credit-parked
writer.

acquire no longer takes a context: the stream's context Done channel is
only closed once the stream is finished (after all ops complete), which
cannot happen while an acquire is parked (the parked write is itself an
in-flight op), so it never fired there. close is the sole abort path,
and every termination route reaches it.

Per-stream only. NOTE: Close/SendError/CloseSend still take the write
lock while holding s.mu, so a send parked on credit can deadlock them;
the lock-ordering rework that makes those paths preempt a parked writer
is handled separately.

Co-Authored-By: roachdev-claude <roachdev-claude-bot@cockroachlabs.com>
@suj-krishnan
suj-krishnan force-pushed the sujatha/flow-control-send-gate branch from 6649f34 to 48cda98 Compare July 22, 2026 05:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants