Drain in-flight streams after a graceful GOAWAY#193
Conversation
41d0950 to
7523894
Compare
| # group of dependent streams by altering the priority of an | ||
| # unused or closed parent stream. | ||
| when :priority | ||
| # An idle stream activated here would never close, keeping a |
There was a problem hiding this comment.
I'm not sure this is correct. according to the comment, PRIORITY frames can be received in the idle or closed stream state. besides, activate_stream will fail if the stream hasn't been created yet, and reprocessing of priority, while it hasn't been implemented yet, may be so in the future. Should be best to keep it as is, don't you think?
There is a bug here though: closed streams won't be available in @streams, so this block will raise an error for closed streams (or worse, create them again?). Therefore, this block should be skipped if stream_id is lower than @last_stream_id and there's no stream in @streams (meaning, it has already been closed). Feel free to give it a go at implementing this, otherwise I can do it post-merge.
There was a problem hiding this comment.
Per §5.1.1 an untracked id ≤ @last_stream_id is already "closed" — nothing to reprioritize. Fresh ids still activate, even while closing; such an idle ghost blocks the terminal :closed, harmlessly (closed? is already true).
…reams A received GOAWAY moves the connection to :closed even when NO_ERROR means streams at or below last_stream_id may still complete (RFC 9113, Section 6.8). In :closed, connection-level WINDOW_UPDATE and SETTINGS are ignored - an in-flight upload stalls once the connection window drains - and escalate to a connection error 15s after the GOAWAY (igrigorik#140), while the peer is still serving the streams. A graceful GOAWAY received while such streams exist now moves the connection to :draining: connection-level frames are processed and the drainable streams - already begun, at or below last_stream_id - run to completion, while frames that would open a new stream (HEADERS for an untracked stream, PUSH_PROMISE, PRIORITY) are discarded and new_stream keeps raising ConnectionClosed. The connection closes once the last drainable stream finishes, or immediately on an error GOAWAY. Streams the GOAWAY refused (above last_stream_id), idle streams, and undelivered push reservations do not hold the drain open. Draining only affects frames the application feeds via receive - nothing is buffered behind its back. Callers gating teardown on closed? should use closed? || draining?. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
const86
changed the title
| when :priority | ||
| # The stream already existed and closed: reprioritizing a | ||
| # closed parent is a no-op here - do not resurrect it. | ||
| next if stream_id <= @last_stream_id |
There was a problem hiding this comment.
| next if stream_id <= @last_stream_id | |
| next if closed? && stream_id <= @last_stream_id |
streams can be initiated by a PRIORITY frame.
There was a problem hiding this comment.
While closing, a resurrected phantom would emit :stream after the GOAWAY and — never completing — wedge the drain in :closing; the reworked test pins both.
| expect(connected_conn).to be_closing | ||
|
|
||
| # Connection-level frames keep being processed while closing. | ||
| connected_conn << f.generate(window_update_frame.merge(stream: 0, increment: 1000)) |
There was a problem hiding this comment.
shouldn't this also be testing that frames for stream do not raise?
There was a problem hiding this comment.
DATA rather than HEADERS or a stream WINDOW_UPDATE: the example also runs as a server, where HEADERS on the even stream id is a connection error, and the helper SETTINGS pin windows at 2³¹-1, so any increment overflows.
|
|
||
| # Response HEADERS racing the local reset must not kill the | ||
| # closing connection. | ||
| client << f.generate(headers_frame.merge(payload: Compressor.new.encode(RESPONSE_HEADERS))) |
There was a problem hiding this comment.
I see you're doing it there. would it be possible to test this all in the same it block?
There was a problem hiding this comment.
One read carries both responses, so the discard-mid-read case covers separate-read delivery too — nothing from the three blocks is lost.
…te specs - PRIORITY for an untracked id at or below the last stream id is skipped only when the connection is closed or closing: live connections keep accepting PRIORITY-initiated streams, while a draining connection is not wedged open nor emits phantom :stream events after a GOAWAY. - Comment why the last closing stream moves the connection to :closed. - The shared drain example also proves stream-addressed frames are processed while :closing. - One client spec block covers delivery to a tracked stream and discard of an untracked one's response HEADERS while closing, same-read. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BqCFwHgYK7fFycnw1xCJde
The comment above the untracked-PRIORITY branch quoted the RFC 7540 priority-tree rationale, which RFC 9113 deprecates and the guard below no longer honors after a GOAWAY; state the actual behavior instead. The README's connection lifecycle section now spells out the closed?/closing? contract: closed? answers "no new work" from GOAWAY on, closing? marks the draining phase, refusals surface via :goaway. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BqCFwHgYK7fFycnw1xCJde
2 participants
On a graceful GOAWAY the connection moved straight to
:closed, where connection-level WINDOW_UPDATE and SETTINGS are ignored — and escalate to a connection error 15s later (#140) — while the peer keeps completing streams at or below last_stream_id (RFC 9113 Section 6.8). In-flight uploads stall on the frozen connection window, and drains longer than 15s kill every promised stream.A graceful GOAWAY received while streams are tracked now enters
:closing: connection-level frames are processed as usual, tracked streams run to completion, and the connection moves to:closedwhen the last one ends (or immediately on an error GOAWAY). HEADERS that would open a new stream and PUSH_PROMISE are discarded (after HPACK decoding, keeping compression state consistent), andnew_streamkeeps raising ConnectionClosed.closed?now answers "does this connection accept new work?" — true from GOAWAY receipt on, exactly when it turned true before this change;closing?distinguishes a still-draining connection. Streams the GOAWAY refused (or that never started) hold nothing open: callers keep tracking refusals via the:goawayevent and retry on a new connection.After a GOAWAY, a PRIORITY frame for an already-used stream id no longer resurrects a phantom stream (previously it re-activated the id and emitted
:streamwith no HEADERS behind it — while draining, such a phantom would also hold the connection in:closingforever). Live-connection PRIORITY handling is unchanged.Draining only affects frames the application feeds via
#receive— nothing is buffered behind its back (the #184 concern). Subsumes the #192 HEADERS loss while draining; #192 still applies to an already-closed connection.Validated end-to-end against golang.org/x/net/http2 graceful shutdown (h2c): session streams keep serving mid-drain, a 3 MB upload crosses the connection window while :closing, and post-GOAWAY stream opens fail fast for retry on a fresh connection.
🤖 Generated with Claude Code