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 session termination #239

Merged
merged 22 commits into from
Nov 26, 2024
Merged

Conversation

ktff
Copy link
Contributor

@ktff ktff commented Nov 11, 2024

Closes #197

This PR adds handling of session termination according to https://datatracker.ietf.org/doc/html/draft-ietf-webtrans-http3#name-session-termination with one exception noted in open questions.

Open questions

  • Protocol requires that the endpoint must reset the send side of all of the streams associated with the session upon learning that the session has been terminated. For that we need to track open streams by storing some handle to them in Worker struct, but all approaches that I see are either not possible at the moment or seem too costly:
    * quinn::SendStream and quinn::RecvStream streams are not Clone so we can't store them directly. This seems to be intentional so it's unlikely for this to be changed.
    * quinn library doesn't expose a way to close a stream with just StreamId so we can't use them. It's maybe possible to get adequate changes into quinn library to enable this.
    * While it's possible to wrap quinn streams in something like Arc<Mutex<_>>, it seems costly.
    @BiagioFesta any idea on how to resolve this? (EDIT: We'll leave it outside this PR)

Todo

@ktff ktff changed the title Session termination Handle session termination Nov 11, 2024
@ktff ktff changed the title Handle session termination Handle connect stream termination Nov 11, 2024
@ktff ktff changed the title Handle connect stream termination Handle session termination Nov 11, 2024
@BiagioFesta
Copy link
Owner

Thank you for this amazing contribution!


@BiagioFesta any idea on how to resolve this?

I don't see any solution on top of my head. As you already correctly mentioned, closing a stream requires mutable access to it, and the stream is "something" left to the application layer.
A chance is to leverage interior mutability with a Mutex, but I'd doubt it is worth it as the connection is closing anyway and streams are bound to that.

I will reason more about it, but definitely is something we can leave outside this PR

@ktff ktff marked this pull request as ready for review November 17, 2024 10:59
Copy link
Owner

@BiagioFesta BiagioFesta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we polish a little bit commits history? (E.g. Finish means finish is a funny commit description, but not very talkative :) )

@ktff ktff force-pushed the session_termination branch from 5b091e2 to 67df9e8 Compare November 25, 2024 06:41
@ktff ktff force-pushed the session_termination branch from 67df9e8 to 090774d Compare November 25, 2024 06:43
@ktff ktff requested a review from BiagioFesta November 25, 2024 06:44
@BiagioFesta BiagioFesta merged commit 26872fd into BiagioFesta:master Nov 26, 2024
8 checks passed
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.

Session not terminating upon closed CONNECT stream
2 participants