Coverage decreased (-0.06%) to 96.651% when pulling 8e6e0d8 on bengreenberg:issue-119 into ebcabd5 on igrigorik:master.

I'm seeing a race condition on pushes of small amounts of data that are canceled by the client. If the push data is small enough and transferred quickly enough, the server may receive a client's RST_STREAM frame after the pushed stream has already been closed. In this case, the http-2 server cannot find the stream id in the @streams hash, so it treats it as as unexpected stream identifier and sends GOAWAY with frame 0, killing the connection. Since the closed push stream is actually valid, I believe this is not correct and the RST_STEAM can be silently ignored in this case.

Makes sense.

Looking into this more, I believe this was introduced with #88 . I don't think the intent of that patch was the delete closed immediately. My fix only deletes the stream if its been closed for 15 seconds or longer.

We do want to remove it from the @streams list of active streams, to keep the accounting correct. We should use existing @streams_recently_closed for what you're after.

We already handle a similar case for push_promises:

To cover the case that you're after, we probably need to look at..

I believe stream accounting is handled by:

      stream.once(:close) do
        @active_stream_count -= 1

Which i kept in with this patch. Does that handle it?

My original patch actually added an if statement around that connection_error line. Unfortunately, that triggered Rubocop to complain about too many layers of nesting, and I think resolving that would require a major code refactor. While exploring other options, I noticed that there is handling in the Stream class for streams in a closed state, so I thought it would be good for frames to still make their way to the actual Stream. Otherwise, I think that code can be removed because I believe it will never be triggered.

Let me know if you think it's still best to do the @streams_recently_closed check before the error is thrown and I will make those changes -- though the refactor is a bit intimidating as I'm new to the codebase.

My original patch actually added an if statement around that connection_error line. Unfortunately, that triggered Rubocop to complain about too many layers of nesting, and I think resolving that would require a major code refactor.

Rubocop is directional advice, we can tell it to shush.. :)

While exploring other options, I noticed that there is handling in the Stream class for streams in a closed state, so I thought it would be good for frames to still make their way to the actual Stream. Otherwise, I think that code can be removed because I believe it will never be triggered.

Ah, good point. Yes that makes sense.

On second pass, I think this looks reasonable 👍

Thanks!