quic: Assert that there are streams available in EnvoyQuicClientSession::OnCanCreateNewOutgoingStream #18786
Conversation
Signed-off-by: Ryan Hamilton <[email protected]>
|
/assign @alyssawilk |
|
Did you try the 2k local runs? Or is this after an upstream fix? |
Signed-off-by: Ryan Hamilton <[email protected]>
Sorry, I converted this to a draft just after I sent it to you because I realized that the ASSERT I changed wasn't where I expected it to be and needed to dig deeper. Ah well :) In any case, back in #18694 we discussed an ASSERT in (Is there a way to un-assign a PR?) |
RyanTheOptimist
changed the title
|
Oh interesting. In #18775 we're calling OnCanCreateNewOutgoingStream() even when there are no new streams available. In that case, maybe this current PR is a non-goal? |
| @@ -101,6 +101,7 @@ void EnvoyQuicClientSession::OnCanCreateNewOutgoingStream(bool unidirectional) { | |||
| return; | |||
| } | |||
| uint32_t streams_available = streamsAvailable(); | |||
| ASSERT(streams_available > 0); | |||
There was a problem hiding this comment.
So I don't think we want this because it's not going to play well with my PR in progress, where we artificially call OnCanCreate... to update the conn pool when we complete handshake even if there's no streams. I alternately we can change that PR to manually onMaxStreamsChanged but I mildly prefer just not adding this assert
There was a problem hiding this comment.
Yeah, I came to that conclusion as well in my previous comment. C'est la vie :) I'll close this out.
quic: Assert that there are streams available in EnvoyQuicClientSession::OnCanCreateNewOutgoingStream now that QUICHE has been fixed.
Signed-off-by: Ryan Hamilton [email protected]
Risk Level: Low
Testing: Existing
Docs Changes: N/A
Release Notes: N/A
Platform Specific Features: N/A