Reset any queued stream on receipt of remote reset

[hawkw]

Fixes #256.

This PR changes state::recv_reset so any closed stream with queued send is immediately reset (and thus, the queue is cleared) on receipt of a RST_STREAM frame from the remote.

This fixes the panic encountered by the test @goffrie posted in #256, where the stream is scheduled to close, receives a RST_STREAM frame, and sets the buffered capacity to 0, but isn't removed from the send queue, so we hit an assertion (or overflow, if debug assertions are disabled) when subtracting a sent frame's size from the buffered size.

I've determined that this does fix the panic in @goffrie's test, but the test instead hangs forever instead of passing. I haven't yet been able to rewrite the test such that it hits the panic on master but doesn't hang when the issue is resolved. I intend to continue working on testing this, but I'm putting this up for review now nonetheless.

[olix0r]

this looks like the only functional change in this PR... can you explain how this resolves the issue?

[carllerche]

+1. It would be good to include a comment explaining this as it is definitely subtle.

[olix0r]

IIUC, the match can be condensed as:

    pub fn recv_reset(&mut self, reason: Reason, queued: bool) {
        match self.inner {
            Closed(..) if !queued => {},
            s => {
                trace!("recv_reset; reason={:?}; state={:?}", reason, s);
                self.inner = Closed(Cause::Proto(reason));
            }
        }
    }

I want to make sure I understand this properly, though. What are the states where we can be closed and have queued data?

https://github.com/carllerche/h2/blob/b1d14caa2251cfa6316d73ad77409fb0be4a41f1/src/proto/streams/state.rs#L73-L87

In particular, I'm not sure if it makes sense to change the closed type when it's already Proto (it's probably harmless? but I'm not sure of the intended behavior). Also, I'm not sure we'd want to override an Io error (though I'm not sure if it's actually possible to receive a reset after an Io error).

[hawkw]

This is fair, I can make the match more specific.

My understanding is that we can have queued data in the following cases:

• when the cause Cause::Scheduled,
• when the cause is Cause::EndStream, which would indicate that we've enqueued an EOS frame but it has yet to be sent (either because there are other frames ahead of it in the send queue, or we just haven't popped it yet)

AFAIK, queued should not be true with any other Cause variants, but I'll change the pattern match to actually encode this nonetheless.

[hawkw]

@olix0r, hopefully 05a6017 is clearer?

[olix0r]

we don't clear the sendq here. Would be clearer as:

... immediately. This causes the send queue to be cleared by prioritize.

(Or whatever is actually correct.

[goffrie]

Hm, there's one more way that we can call clear_queue, which is via recv_err; I believe this is able to cause a similar issue wherein the stream's state doesn't necessarily become is_peer_reset and so the check here doesn't trigger. For example I've seen this happen when receiving a GO_AWAY frame, which has caused this assertion to trip because the frames were all cleared.

[hawkw]

@goffrie Ah, thanks for pointing that out --- I think we'll want to add similar logic to recv_error for when a stream has frames in the send queue. I'll look into that in an additional PR.

[carllerche]

Thanks for getting this done.