fix: notify_recv after send_reset() in reset_on_recv_stream_err(), to ensure local stream is released properly

[jiahaoliang]

Similar to what have been done in fn send_reset(), we should notify RecvStream that is parked after send_reset().

To fix an issue where if RecvStream is parked, server doesn't release the StreamRef properly after reset_on_recv_stream_err() and as a result Stream is never removed.

[jiahaoliang]

Hey @seanmonstar , hope you're doing well! When you have a moment, could you take a look at my PR. The change itself is self-explanatory. Let me know if anything needs tweaking. Thanks a lot for your time.

[seanmonstar]

Looks like this aimed at 0.3.x, should be master?

[jiahaoliang]

We identified this issue on h2 0.3.26 as we are still working with hyper 0.14.30. I am not sure whether master is affected or not. Looks like the pipeline isn't compatible with 0.3.x?

[seanmonstar]

The 0.3.x branch doesn't see active development, so sometimes the checks get out of date (we only really backport important bug fixes). I've fixed up 0.3.x, if you want to rebase. I can port this to fix to master, too.

[jiahaoliang]

The 0.3.x branch doesn't see active development, so sometimes the checks get out of date (we only really backport important bug fixes). I've fixed up 0.3.x, if you want to rebase. I can port this to fix to master, too.

Please do. I’ve rebased this PR too. Could you do me a favor and trigger the pipeline again? Hopefully it should be ready for merge.

[seanmonstar]

Looks like a test is now running forever.

[jiahaoliang]

Hey @seanmonstar , I need some help. In the test recv_next_stream_id_updated_by_malformed_headers,

1. client sent a bad frame with stream id 1
2. received Reset(PROTOCOL_ERROR) from server
3. sent a good frame with same stream id 1
4. expects GoAway(PROTOCOL_ERROR) from server <-- where it stucks after the change.

I analysed the traces before and after the change (attached as 'good_trace.txt' and 'bad_trace.txt'), it seems the key differences are

• in the good_trace, when good frame comes, stream 1 had already been removed from the server store. A new recv.open() was triggered and the server found the stream id 1 was less then next_stream_id(), so it issued a GoAway(PROTOCOL_ERROR).
• in the bad trace, when good frame comes, stream 1 had beed pushed in pending_reset_expired queue. A new recv.open() was not triggered as it found the stream id from the store. Then the server hit the following logic without issuing a GoAway

src/proto/streams/streams.rs:455
        if stream.state.is_local_error() {
            // Locally reset streams must ignore frames "for some time".
            // This is because the remote may have sent trailers before
            // receiving the RST_STREAM frame.
            tracing::trace!("recv_headers; ignoring trailers on {:?}", stream.id);
            return Ok(());
        }

I am not sure how to fix this. Other tests and h2spec were passed.

good_trace.txt
bad_trace.txt

[jiahaoliang]

Hi @seanmonstar , could you provide some feedback or advice on my PR when you have a moment? Really appreciate your insights!

[seanmonstar]

Sorry for taking a bit to respond, I've looked into the failing test now. I pushed a fix in #816. I can squash that in once CI runs.

[seanmonstar]

Ok, this other PR was merged. Thanks for submitting the fix!