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

Mark WebTransport streams keep-alive #1369

Open
ddragana opened this issue Oct 6, 2022 · 2 comments · May be fixed by #1389
Open

Mark WebTransport streams keep-alive #1369

ddragana opened this issue Oct 6, 2022 · 2 comments · May be fixed by #1389
Assignees
Labels
p1 a bug that needs to be fix soon

Comments

@ddragana
Copy link
Contributor

ddragana commented Oct 6, 2022

We need to propagate neqo_transport::Connection to this function and call this function:

// TODO conn.stream_keep_alive(stream_id, true)?;

@ddragana ddragana added this to Needs triage in WebTransport via automation Oct 6, 2022
@ddragana ddragana moved this from Needs triage to High priority in WebTransport Oct 6, 2022
@ddragana ddragana changed the title Mark WebTranposrt streams keep-alive Mark WebTransport streams keep-alive Oct 6, 2022
@ddragana ddragana added the p1 a bug that needs to be fix soon label Oct 6, 2022
@MayyaSunil MayyaSunil self-assigned this Nov 28, 2022
@MayyaSunil
Copy link
Collaborator

MayyaSunil commented Dec 1, 2022

I tried propagating neqo_transport::Connection and calling conn.stream_keep_alive(stream_id, true)?. This caused panic for the following tests in streams.rs.

wt_client_stream_uni
wt_server_stream_uni

The panic is originating from RecvStreams::keep_alive:

 pub fn keep_alive(&mut self, id: StreamId, k: bool) -> Res<()> {             
  65         let self_ka = &mut self.keep_alive;                                      
  66         let s = self.streams.get_mut(&id).ok_or(Error::InvalidStreamId)?;                                                                                                                                                                                            
  

FWIU, we shouldnt enable the keep alive flags for local unidirectional streams as they do not have recv streams associated with the session. Hence, we should enable keep alive flag for the following streams:

  • Bidirectional - local and remote
  • Unidirectional - remote

@KershawChang, @martinthomson : Please confirm if my understanding is correct

@MayyaSunil MayyaSunil linked a pull request Dec 1, 2022 that will close this issue
@martinthomson
Copy link
Member

@MayyaSunil you should only need to enable a keep-alive for the session stream, that is the "request" that is still open. If that stream closes, the whole WebTransport session ends, so - that way - you will be keeping the connection alive as long as there is any WebTransport session active.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p1 a bug that needs to be fix soon
Projects
WebTransport
High priority
Development

Successfully merging a pull request may close this issue.

3 participants