Skip to content

Ignore leftover frames received after the connection is closed - #1324

Open
adarshx01 wants to merge 1 commit into
python-hyper:masterfrom
adarshx01:fix/ignore-frames-after-close
Open

adarshx01 wants to merge 1 commit into
python-hyper:masterfrom
adarshx01:fix/ignore-frames-after-close

Conversation

@adarshx01

Copy link
Copy Markdown

Fixes #1199.

After GOAWAY (or any other path into CLOSED), the TCP receive buffer can still hold PING/DATA/SETTINGS frames. Those currently miss the CLOSED transition table and raise ProtocolError. Extra GOAWAY frames stay valid, matching the existing multi-GOAWAY tests; everything else is dropped and does not generate a PING ACK.

@feiiiiii5

Copy link
Copy Markdown

Checked the behaviour rather than only the added test, and it matches what the description and CHANGELOG claim. RFC 9113 does not contradict it, which I also wanted to confirm before spending more time here.

The RFC anchor holds. There is no connection state machine section in RFC 9113 — §5.1 is stream states only — so the relevant text is §5.4.2, which says that after RST_STREAM "the peer ... MUST be prepared to receive any frames that were sent or enqueued for sending by the remote peer. These frames can be ignored, except where they modify connection state". §6.8 only forbids opening new streams after GOAWAY; it does not require a connection error for frames still in flight. So dropping them is defensible, and §5.4.2 is the precedent for it.

All nine frame types, not just the one in the test. I drove a server connection into ConnectionState.CLOSED with a GOAWAY and then fed one frame of each type, using hyperframe to build them:

DATA  HEADERS  PRIORITY  RST_STREAM  SETTINGS  PUSH_PROMISE  PING  WINDOW_UPDATE
   -> 0 events, 0 bytes written back, for all eight
GOAWAY -> still processed, 1 ConnectionTerminated event

So the guard does cover the whole dispatch table, not just the leftovers the test happens to use. Two specifics worth stating because they are easy to get wrong: a PING in this state gets no ACK, which matches the description, and a further GOAWAY is still processed rather than dropped, which is what keeps the existing multi-GOAWAY tests valid. Calling receive_data() repeatedly afterwards is also safe.

Suite and coverage. 1663 passed on this branch against 1662 on master, so the PR is purely additive test-wise. src/h2/connection.py stays at 100% (657 statements, 154 branches), so fail_under=100 holds with the new early return.

One note on my own tooling rather than the PR: my first two attempts at this matrix produced two false positives — a GOAWAY with a zero length field, and a WINDOWUpdate whose increment I set on the wrong attribute name, which serialised as 0 and was rejected as non-compliant before ever reaching your guard. Both looked like gaps in the fix. Building the frames with hyperframe instead of by hand removed the whole class of error, so if you extend the test to more frame types that is probably the cheaper route.

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.

ProtocolError on receive_data after connection is closed

2 participants