Reset queued streams on recv_err

[hawkw]

See #258 (comment) --- the same changes made in recv_reset in #258 should also probably be made in recv_err.

[goffrie]

I don't think this will work as-is: recv_reset sets the state to Closed(Cause::Proto(_)), which lets the stream be considered is_peer_reset - but here we use a different Cause.

[carllerche]

Can a test be added demonstrating the fix? Also, I think @goffrie's comment is correct.

To be honest, I find it very strange that recv_err sets the closed state to LocallyReset. I know that this is what the existing code does, but I have not untangled why it does this...

[carllerche]

It looks like @seanmonstar added LocallyReset in #174, maybe he can provide some context here?

[seanmonstar]

LocallyReset is when we are telling the remote about a RST_STREAM. The reason to remember that we sent the reset is so that if we receive any frames shortly after having sent the RST_STREAM, we want to just discard them, instead of sending a GOAWAY, because the frames may have already been en-route.

I understand recv_err is called when we are decoding a frame, or after decoding and trying to assign it to an active stream, and find an error. We're going to be sending the RST_STREAM, so I think it's right that we should store LocallyReset in the cause.

[carllerche]

If #261 is merged, it would also fix the issue this PR is trying to fix.

(though, a test would still be 👍 )

[hawkw]

@carllerche et al, I think moving forward with #261 is the correct way to solve this issue, which is why I haven't touched this PR since that branch opened.

[carllerche]

Ok, #261 was merged. As described there, the failure case is not easy to write up in a test and any attempt would be very brittle as it relies on a particular sequence of ready / not ready results from write. The fuzzer catches the case pretty quickly, so I would say that no explicit test is required.