Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deal with RST_STREAM in HalfClosedLocal state #107

Merged
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 19 additions & 8 deletions Network/HTTP2/H2/Receiver.hs
Original file line number Diff line number Diff line change
Expand Up @@ -557,21 +557,32 @@ stream FrameRSTStream header@FrameHeader{streamId} bs ctx s strm = do
ConnectionErrorIsSent EnhanceYourCalm streamId "too many rst_stream"
RSTStreamFrame err <- guardIt $ decodeRSTStreamFrame header bs
let cc = Reset err
closed ctx strm cc

-- The spec mandates (section 8.1):
-- HTTP2 spec, section 5.1, "Stream States":
--
-- > A stream in the "open" state may be used by both peers to send frames
-- > of any type. (..) From this state, either endpoint can send a frame
Copy link
Owner

Choose a reason for hiding this comment

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

What does (..) mean?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh, (..) is just a standard way of indicating that some text was omitted. In this case, I only showed part of the spec that was relevant.

-- > with an END_STREAM flag set, which causes the stream to transition into
-- > one of the "half-closed" states. An endpoint sending an END_STREAM
-- > flag causes the stream state to become "half-closed (local)"; an
-- > endpoint receiving an END_STREAM flag causes the stream state to become
-- > "half-closed (remote)".
--
-- > When this is true, a server MAY request that the client abort
-- > transmission of a request without error by sending a RST_STREAM with an
-- > error code of NO_ERROR after sending a complete response (i.e., a frame
-- > with the END_STREAM flag).
-- Crucially (for the specific case we're dealing with here), it continues:
--
-- We check the first part ("after sending a complete response") by checking
-- the current stream state.
-- > /Either endpoint/ can send a RST_STREAM frame from this state, causing
-- > it to transition immediately to "closed".
--
-- (emphasis not in original). This justifies the two non-error cases,
-- below. (Section 8.1 of the spec is also relevant, but it is less explicit
-- about the /either endpoint/ part.)
case (s, err) of
Copy link
Owner

Choose a reason for hiding this comment

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

In the case of _otherwise, closed is called to delete the stream from the stream table.
I'm not sure but I guessclosed should be called in any cases.
So, closed should be moved to the outside of case.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Makes sense. I guess I introduced that bug as part of #78, apologies. I've pushed a fix, but let me run my full test suite first before merging.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok, all tests pass ✔️

(Open (Just _) _, NoError) -> -- HalfClosedLocal
return (Closed cc)
(HalfClosedRemote, NoError) ->
return (Closed cc)
_otherwise -> do
closed ctx strm cc
E.throwIO $ StreamErrorIsReceived err streamId

-- (No state transition)
Expand Down
Loading