Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle zombie streams, when sending rst #76

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

martenrichter
Copy link

If QuicSession::OnStreamClosed is called on a stream with a pending outgoing fin, a zombie stream will be created. A call to QuicStream::OnStopSending afterward, which in turn calls QuicStream::MaybeSendRstStream should kill the zombie finally. But this route misses a call to session's session_->MaybeCloseZombieStream, so the stream is not removed from active streams. This triggers a quic_bug_12435_2 when OnConnectionClosed is finally called on session close. This commit adds the missing call to the MaybeCloseZombieStream.

If QuicSession::OnStreamClosed is called on a stream with a pending outgoing fin, a zombie stream will be created. A call to QuicStream::OnStopSending afterward, which in turn calls  QuicStream::MaybeSendRstStream should kill the zombie finally. But this route misses a call to session's session_->MaybeCloseZombieStream, so the stream is not removed from active streams.  This triggers a quic_bug_12435_2 when OnConnectionClosed is finally called on session close. This commit adds the missing call to the MaybeCloseZombieStream.
@yangfanud
Copy link
Collaborator

Thank you for reporting the issue!
To make sure I understand the scenario correctly:

QuicSession::OnStreamClosed is called on a stream with a pending outgoing fin
Do you mean the stream has consumed fin, and sent fin but waiting for all data get acknowledged?
QuicStream::OnStopSending afterward
Is a STOP_SENDING frame gets received for this stream?

@martenrichter
Copy link
Author

Thank you for reporting the issue! To make sure I understand the scenario correctly:

QuicSession::OnStreamClosed is called on a stream with a pending outgoing fin
Do you mean the stream has consumed fin, and sent fin but waiting for all data get acknowledged?
QuicStream::OnStopSending afterward
No the other way round, a fin is outstanding but all data is received. And with fin outstanding I mean the the flag which is checked here:

bool QuicStream::IsWaitingForAcks() const {

Test code is on the firefox side:
https://github.com/fails-components/webtransport/blob/0bbe8abd1cc9016d902812823f9ac5a502f4385a/test/stream-limits.spec.js#L117
and on the server side.

Is a STOP_SENDING frame gets received for this stream?
I do not know. I have just put print outs on all code points, that change variables checked at

bool QuicStream::IsWaitingForAcks() const {

and I have seen that a to QuicStream::OnStopSending change the variables, so that the Zooombietest reports false and that on this route session_->MaybeCloseZombieStream, was not called.

@yangfanud
Copy link
Collaborator

Thanks for the details. In addition to your PR in flight, it is a bug that session does not deliver RESET or STOP_SENDING to zombie streams. I am going to land a fix internally, if that sounds good to you?
Again, thanks for reporting the issue!

@martenrichter
Copy link
Author

Yes, this sounds good. All I care about is that it is fixed in quiche.

copybara-service bot pushed a commit that referenced this pull request Sep 22, 2024
This is to fix a bug that a zombie stream is not closed when receiving STOP_SENDING. Credit to external report in #76

Protected by FLAGS_quic_reloadable_flag_quic_deliver_stop_sending_to_zombie_streams.

PiperOrigin-RevId: 677548901
@yangfanud
Copy link
Collaborator

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants