Do not remove closed streams immediately

[bengreenberg]

Closed streams need to remain in the Connection @streams list so that
frames received for closed streams do not cause a protocol error.
They are cleaned up if closed for 15 seconds when another stream is
closed.

Fixes #119

[coveralls]

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

[igrigorik]

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:

http-2/lib/http/2/connection.rb

Lines 273 to 290 in 0aeaac2

	# PUSH_PROMISE frames MUST be associated with an existing, peer-
	# initiated stream... A receiver MUST treat the receipt of a
	# PUSH_PROMISE on a stream that is neither "open" nor
	# "half-closed (local)" as a connection error (Section 5.4.1) of
	# type PROTOCOL_ERROR. Similarly, a receiver MUST treat the
	# receipt of a PUSH_PROMISE that promises an illegal stream
	# identifier (Section 5.1.1) (that is, an identifier for a stream
	# that is not currently in the "idle" state) as a connection error
	# (Section 5.4.1) of type PROTOCOL_ERROR, unless the receiver
	# recently sent a RST_STREAM frame to cancel the associated stream.
	parent = @streams[frame[:stream]]
	pid = frame[:promise_stream]
	# if PUSH parent is recently closed, RST_STREAM the push
	if @streams_recently_closed[frame[:stream]]
	send(type: :rst_stream, stream: pid, error: :refused_stream)
	return
	end

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

http-2/lib/http/2/connection.rb

Lines 331 to 333 in 0aeaac2

	# An endpoint that receives an unexpected stream identifier
	# MUST respond with a connection error of type PROTOCOL_ERROR.
	connection_error

[bengreenberg]

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.

[igrigorik]

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 👍

[igrigorik]

Thanks!