cc @arjan-bal @dfawley

Hi @alanprot, thanks for sharing the test to repro. I haven't debugged the test, but wanted to ask if you know why the check added causes stream deletion to be skipped? From a look quick look at the places streamDone is being set, I found only two:

In both places, deleteStream is called eventually. Do you know how the code ends up in a situation where the stream is marked as done, but the it's not deleted from the map?

Edit: It seems like the onWrite callback may not be getting executed.

Hi @arjan-bal

As far i could see the problem is we add set the clean and add the hdr on the controlbuff at

And we get to:

And at this point, the stream is not on the activeStreams list, so processData returns here:

And i think this stream never become added back on the activeStreams list, so we never clean it up.

So it seems that it only happens when the ServerSide is sending tons of data and waiting for quota?

Thank you @alanprot, I think I understand the sequence of events leading to this.

1. The server stream exhausts the stream quota, and starts buffering data frames.
2. The server attempts to finish the RPC gracefully by sending trailers by calling finishStream.
3. The write of the header frame gets blocked due to the pending data frames that are awaiting flow control quota.
4. Once the deadline expires, the server's deadline monitoring timer tries to call closeStream, which is a no-op since the stream state is already marked streamDone by finishStream in point 1.
5. The client sends and RST stream, the server calls closeStream which is again a no-op.

The simplest fix is to remove the guard block added in closeStream in #8071. We may end up pushing redundant cleanupStream events on to the controlbuf, but it doesn't cause correctness issues since cleanupStreamHandler returns early in such cases.

@dfawley what do you think?

The simplest fix is to remove the guard block added in closeStream in #8071. We may end up pushing redundant cleanupStream events on to the controlbuf, but it doesn't cause correctness issues since cleanupStreamHandler returns early in such cases.

That makes sense to me. I can't think of any other way to do this. Does the transport properly handle the rest of this situation, especially: dropping the queued data frames when the RST_STREAM is sent (or shortly thereafter)?

Also maybe we wanna consider decrementing this metric only if the stream was present on the activeStreams map?

That makes sense to me. I can't think of any other way to do this. Does the transport properly handle the rest of this situation, especially: dropping the queued data frames when the RST_STREAM is sent (or shortly thereafter)?

All the pending Data and Header frames are dropped in cleanupStreamHandler.

When does our max incoming streams quota go back up?

The check for max concurrent streams depends on the size of the activeStreams map.
We delete from activeStreams map only in deleteStream().
So the question is: When is deleteStream() called?

Without timeout:
After the trailers are written, at the beginning of cleanupStreamHandler() due to cleanupStream.onWrite().

With timeout:
Just before cleanupStream event is pushed into the controlBuf. So there is a small interval before the cleanupStreamHandler is called when an extra stream could be started. We can minimize this interval this by using cleaupStream.onWrite(), similar to the timeout case above, but this is not super urgent since it's been this way for a while.

@alanprot are you interested in raising a PR with the fix and a unit test for catching regressions?

@arjan-bal I can definitely do that, but I’m out until Tuesday, so I’d only be able to pick it up then. :)

I'll be closing this PR and using #8517 to track the fix as per our normal bug fixing process. When the PR with the fix is merged #8517 can be marked as completed.