When Streams are dropped, close Connection (#221) #222
Conversation
When all Streams are dropped / finished, the Connection was held open until the peer hangs up. Instead, the Connection should hang up once it knows that nothing more will be sent. To fix this, we notify the Connection when a stream is no longer referenced. On the Connection poll(), we check that there are no active, held, reset streams or any references to the Streams and transition to sending a GOAWAY if that is case. These changes cause the example to work as expected (exiting when finished getting the response), but break other tests that are not expecting the connection to hang out.
|
The PR looks good at a glance, but there is a complication. Re the tests, I pushed a commit (7613406) that includes a fix for some of the tests. The issue with the client tests is that they drop the Now, the complication. It turns out that the server half uses the I think what will need to happen is the logic to actually call This relates to #69 |
|
Thanks for the examples on how to fix the tests, I pushed a commit fixing the rest of the tests as well as splitting the logic between Let me know what you think! |
| @@ -204,6 +204,32 @@ fn sends_reset_cancel_when_req_body_is_dropped() { | |||
| srv.join(client).wait().expect("wait"); | |||
| } | |||
|
|
|||
| #[test] | |||
| fn sends_goaway_when_serv_closes_connection() { | |||
There was a problem hiding this comment.
I know you didn't originally intend to add graceful shutdown to the server, but it looks like this adds it! It'd be bonus points if there could be another test showing multiple requests being sent and accepted, and then close_connection() called before the responses (and their bodies?) are finished, that the outstanding streams can finish before the connection terminates.
(I suppose we'd also need to do something about any new streams sent after sending the goaway, I don't recall what the spec says to do. REFUSED_STREAM?)
There was a problem hiding this comment.
The spec is slightly confusing on this matter. In one case, it says:
Receivers of a GOAWAY frame MUST NOT open additional streams on the connection
But it doesn't mention a protocol error. It seems to suggest those new streams can simply be discarded:
After sending a GOAWAY frame, the sender can discard frames for streams initiated by the receiver with identifiers higher than the identified last stream
With a caveat that data frames still need to count towards congestion flow control.
There was a problem hiding this comment.
I added a test for showing a request being accepted, then the server closing the connection on the next request (and subsequently ignoring the third request).
Not sure how to make sure any properties like congestion flow control still hold true, please let me know if any changes need to be made!
|
Are there any other changes required? |
|
Yikes, I forgot about this, sorry! I'll take a look again. |
src/proto/streams/counts.rs
Outdated
| @@ -46,6 +46,10 @@ impl Counts { | |||
| self.peer | |||
| } | |||
|
|
|||
| pub fn has_streams(&self) -> bool { | |||
| self.num_send_streams != 0 || self.num_recv_streams != 0 || self.num_reset_streams != 0 | |||
There was a problem hiding this comment.
This probably doesn't need to check num_reset_streams, as that's just a count of "stream metadata we are holding on to temporarily to allow us to ignore incoming frames on a stream we just reset". The count is to just allow us to cap how many of those extra resources we keep in memory, but shouldn't affect if the connection is done, I believe.
src/proto/streams/streams.rs
Outdated
| // of canceling the stream), we should notify the task | ||
| // (connection) so that it can close properly | ||
| if stream.ref_count == 0 && stream.is_closed() { | ||
| if let Some(ref task) = actions.task.as_ref() { |
There was a problem hiding this comment.
This should probably be taken (actions.task.take()) as well. At least, that's the pattern that has been done through the lib whenever notifying on an Option<Task>.
|
LGTM, I haven't noticed anything besides what @seanmonstar has called out. |
|
Updated with all your feedback, let me know! |
src/proto/streams/counts.rs
Outdated
| @@ -46,6 +46,10 @@ impl Counts { | |||
| self.peer | |||
| } | |||
|
|
|||
| pub fn has_streams(&self) -> bool { | |||
| self.num_recv_streams != 0 || self.num_reset_streams != 0 | |||
There was a problem hiding this comment.
I think it was num_reset_streams that was supposed to be ignored? num_send_streams is important. Is that correct @seanmonstar?
There was a problem hiding this comment.
Yes, it is num_reset_streams that isn't needed.
There was a problem hiding this comment.
Oh crap, my bad. Updated.
|
CI passed 🎊 |
When all Streams are dropped / finished, the Connection was held
open until the peer hangs up. Instead, the Connection should hang up
once it knows that nothing more will be sent.
To fix this, we notify the Connection when a stream is no longer
referenced. On the Connection poll(), we check that there are no
active, held, reset streams or any references to the Streams
and transition to sending a GOAWAY if that is case.
These changes cause the example to work as expected (exiting when finished getting the response), but break other tests that are not expecting the connection to hang out.