ignore management frames for a period after closing a connection #140
Conversation
…osing the connection
…es and promptly ignore them after officially closing
HoneyryderChuck
changed the title
|
@igrigorik I went with a similar approach with timers. IMO the connection could profit from having a single state transition point of entry (ex: |
| emit(:goaway, frame[:last_stream], frame[:error], frame[:payload]) | ||
| when :altsvc, :blocked | ||
| emit(frame[:type], frame) | ||
| else | ||
| connection_error | ||
| end | ||
| when :closed | ||
| connection_error if (Time.now - @closed_since) > 15 |
There was a problem hiding this comment.
Overall, what you wired up here makes sense. 👍
One question I have is.. Current logic will cover {settings, window_update, ping, goaway} connection management frames. What about other types of frames, do we need to do anything there? Per #139 (comment)...
After sending a GOAWAY frame, the sender can discard frames for
streams initiated by the receiver with identifiers higher than the
identified last stream. However, any frames that alter connection
state cannot be completely ignored. For instance, HEADERS,
PUSH_PROMISE, and CONTINUATION frames MUST be minimally processed to
ensure the state maintained for header compression is consistent (see
Section 4.3); similarly, DATA frames MUST be counted toward the
connection flow-control window. Failure to process these frames can
cause flow control or header compression state to become
unsynchronized.
Anything else / special we need to think about with respect to the above?
There was a problem hiding this comment.
Fair enough. One corner case here is that we'll be ignoring all frames, irrespective of their ID being established before or after GOAWAY is emitted, but that's probably OK. If that surfaces as an issue, we can revisit.
There was a problem hiding this comment.
That is true, and I thought about that. One should throw protocol error for when a stream id is higher than the last stream id from the goaway frame. But I thought that was out of scope. Am I wrong?
There was a problem hiding this comment.
I think it's worth leaving a comment in the source, stating that we're not enforcing this behavior. I don't think we need to block this PR on implementing that though.
There was a problem hiding this comment.
after reading better, I think I advised for something which is not RFC-compliant. I think this PR covers everything necessary.
spec/connection_spec.rb
Outdated
| @@ -542,6 +542,13 @@ | |||
| expect { @conn.new_stream }.to raise_error(ConnectionClosed) | |||
| end | |||
|
|
|||
| it 'should not raise error when receiving connection management frames after closing' do | |||
There was a problem hiding this comment.
"should not raise error when receiving connection management frames immediately after emitting goaway"?
Any merit in wiring up tests for one or two other types of frames?
There was a problem hiding this comment.
just added.
Will fix #139 .