search

ninenines / cowlib

Support library for manipulating Web protocols.
ISC License
281 stars 172 forks source link

Handle last remote stream ID in cow_http2_machine #109

Closed zuiderkwast closed 3 years ago

zuiderkwast commented 3 years ago

New function set_last_streamid/1 sets the last accepted stream ID to the last known remote stream ID. Frames with a remote stream ID greater than this are thereafter ignored by frame/2, which returns {ok, Http2Machine} for such frames.

zuiderkwast commented 3 years ago

I have tested this locally against a modified version of the Graceful Shutdown branch of Cowboy.

essen commented 3 years ago

You need to ignore (at least some) frames after all the checks, not before. For example if you ignore HEADERS where you currently do, it will not update the HPACK state, and as a result if trailers come in for a legitimate stream the state will not be valid.

zuiderkwast commented 3 years ago

Of course. :facepalm: I assumed HPACK and flow control was done in some earlier stage (e.g. while parsing), but of course it's in the machine. I've updated now. Ignored DATA frames are counted towards the connection flow-control window. Frames that affect the HPACK state are processed and then the result is discarded afterwards.

zuiderkwast commented 3 years ago

Bump.

I updated this a week ago. Do you have time to take a look soon?

essen commented 3 years ago

After Gun 2.0.0-rc.1.

zuiderkwast commented 3 years ago

In general I don't think we need to modify the *_frame functions. The PR should act on the returned value from frame/2 only. I think we should just wrap the result and, depending on the frame, transform the returned tuple into a plain {ok, State}. Errors should be returned regardless.

So, I'll apply maybe_discard_result/1 on the result regardless of frame type.

Then maybe_discard_result/1 needs to allow the result {ok, {goaway, LastStreamID, ...}, State} regardless of LastStreamID, which is the only tuple which doesn't follow the pattern that, because the StreamID is a LastStreamID (such as 0x7fffff) and not an actual stream ID.

So I think you're right that data_frame/2, rst_stream_frame/2 and window_update_frame/2 (the one with a stream ID) don't need any changes as long as the result is discarded afterwards. (The #http2_machine{} state contains the discarded streams so we won't have 'unknown stream' errors and such.)

Not sure about send.

Send can be triggered by window_update_frame/2, but I don't think there can ever be any buffered data to send for streams which are ignored in this way, so I think we don't need to add any logic for send results.

essen commented 3 years ago

OK that looks better I'll do a double check this coming week. Thanks!

essen commented 3 years ago

Please update https://github.com/ninenines/cowboy/pull/1471 as well so I can test that it all works as intended.

essen commented 3 years ago

Merged, thanks!