OnCanCreateNewOutgoingStream in SpdySession

[martenrichter]

Hi,
I am trying to make unit test for the MAX_STREAM property in my WebTransport lib.
I ran into an issue, if the number of bidirectional streams is increased the visitor was not informed.
I tracked it down to

quiche/quiche/quic/core/http/quic_spdy_session.cc

Line 1619 in 87cb8c8

void QuicSpdySession::OnCanCreateNewOutgoingStream(bool unidirectional) {

which does not inform in case of bidrectional streams.
The new generic session does it:

quiche/quiche/quic/core/quic_generic_session.cc

Line 170 in 87cb8c8

void QuicGenericSessionBase::OnCanCreateNewOutgoingStream(bool unidirectional) {

It is not a big deal, since I can overload the method, but I am wondering, if it is intentional and if I am missing something.

[martenrichter]

I have to correct myself; I can not just override the virtual, these session objects are two separate things. But still, the QuicSpdySession, where the code where the call ends after receiving an update. So, I am stuck here and grateful for a pointer on how to pass "OnCanCreateNewOutgoingStream" onto the WebTransportSessionVisitor object.
The "QuicGenericSessionBase" misses the Http3 parsing stuff and can not be used.

[martenrichter]

Cross linking related chromium bug:
https://bugs.chromium.org/p/chromium/issues/detail?id=1337606&q=webtransport&can=1

[martenrichter]

OK, I could fix it with this:
fails-components/webtransport@4de5fa1
However, it is more a workaround to solve it also in Chromium,
one should go to OnCanCreateNewOutgoingStream in SpdySession and iterate through all its streams, check if it is a WebTransport session stream, and call OnCanCreateNewOutgoingStream.
And, of course, OnCanCreateNewOutgoingStream should be added to the WebTransportSession Object, and the implementation then calls its visitors.

[martenrichter]

In order to see what the issue is, you can look into the test I have written:
fails-components/webtransport@d3b5ff6
Basically the client creates 150 bidi streams and exhausts the limit of 100 streams (including the session stream, which would be different for http/2). Then it triggers by opening a unidirectional stream (it is misuse as a cheap communication channel), the closure of 51 streams on the server side, which allows the client side to finish the opening of the remaining 51 streams.
However, without a passing OnCanCreateNewOutgoingStream the client side would not progress with the opening as it did not know that it is possible to open streams again. (Although creating another bidirectional stream would have triggered it).
So the tests starved in this case. This is not a very severe bug, but one prone to very bogus race conditions.

[vasilvv]

Hi Marten,

You're correct that this is a bug (I'll need to fix this). I think this shouldn't affect the production Chrome APIs, since those are defined to immediately fail whenever you exceed the stream limit, and we don't currently have a way to notify the application that new streams are available (it was recently added to the spec, but isn't implemented in our code yet).

[martenrichter]

Hi Victor,
ok, yes if Chrome immediately fails, then the deadlock can not occur.
But of course, if you implement the new feature, then it will. (So what I am currently doing.)