Skip to content

Conversation

@HoneyryderChuck
Copy link
Collaborator

while it's valid to process ping frames after sending/receiving goaway frames, preparing an AC frame may inadvertedly put bytes on the user buffer, thereby signaling that there's something to write back instead of terminating the connection,\

@igrigorik @mullermp would like to ask you guidance on this one. The RFC is not specific on what to do when receiving a PING frame after receiving a GOAWAY frame with stream id 0. I found at least one instance of a server which does exactly that, which led to some corruption on a long lived client connection, and while I can theoretically work around it on the client side in order to wipe out the buffer when processing a ping ack frame on a closed connection, that seems to be redundant and something the parser should do for me.

while it's valid to process ping frames after sending/receiving goaway frames, preparing an AC frame may inadvertedly put bytes on the user buffer, thereby signaling that there's something to write back instead of terminating the connection
@HoneyryderChuck HoneyryderChuck force-pushed the fix-ping-frame-when-closed branch from 8cdb4b7 to dd1e475 Compare October 14, 2025 09:00
this causes the truffleruby build to fail, as it expects to either receive a ping ack or have the connection closed
@HoneyryderChuck HoneyryderChuck force-pushed the fix-ping-frame-when-closed branch from d4e74e9 to 33054ed Compare October 14, 2025 09:40
@mullermp
Copy link
Collaborator

mullermp commented Oct 14, 2025

I'm definitely not a subject matter expert in HTTP2 but taking a dive into the specification, I think this is wrong.

Receivers of a PING frame that does not include an ACK flag MUST send
a PING frame with the ACK flag set in response, with an identical
payload. PING responses SHOULD be given higher priority than any
other frame.

I read this as - you must respond back to PING regardless of the situation and it should be at highest priority (e.g. it supersedes the GOAWAY frame).

Is there a different or underlying fix that can be made to prevent those extra bytes or perhaps it is just the responsibility of the client to flush those? I think that after GOAWAY we can't just assume the connection is closed and done, we have to continue to do some house keeping until it's fully shut down.

@HoneyryderChuck
Copy link
Collaborator Author

thx @mullermp ! I also read that section, and where I'm a bit on the fence here is on what to do when the receiver sees the frames in that particular order:

  1. client reads payload to buffer, parses and processes each frame in order
  2. client receives GOAWAY frame from server, shuts down the socket
  3. client receives PING frame (now what?)

While we definitely have that issue at the frame emission level (http-2 does not support prioritisation, i.e. it yields the frames per order), I'm just not sure what's the correct approach in the sequence of events above. should the parser somehow eager-parse, then reorder by priority, then yield to the caller?

@mullermp
Copy link
Collaborator

client receives GOAWAY frame from server, shuts down the socket

I think this is the fundamental problem. GOAWAY means to stop creating new streams. When the client receives GOAWAY, instead of shutting down the socket, the connection should be moved to a closing state. Then process the rest of the control frames (ping ack) then shut down the socket.

Reading GOAWAY:

The GOAWAY frame (type=0x7) is used to initiate shutdown of a
connection or to signal serious error conditions. GOAWAY allows an
endpoint to gracefully stop accepting new streams while still
finishing processing of previously established streams. This enables
administrative actions, like server maintenance.

phrases such as "initiate shutdown" and "gracefully stop" and "administrative actions" I think indicate that we should not be immediately terminating the connection on that frame.

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