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

Add a mechanism to close a session with an error message #60

Merged
merged 16 commits into from Oct 19, 2021

Conversation

vasilvv
Copy link
Collaborator

@vasilvv vasilvv commented Jul 29, 2021

Fixes #41.

draft-ietf-webtrans-http3.md Outdated Show resolved Hide resolved
draft-ietf-webtrans-http3.md Show resolved Hide resolved
draft-ietf-webtrans-http3.md Outdated Show resolved Hide resolved
draft-ietf-webtrans-http3.md Outdated Show resolved Hide resolved
draft-ietf-webtrans-http3.md Outdated Show resolved Hide resolved
Upon learning about the session being terminated, the endpoint MUST stop
sending new datagrams and reset all of the streams associated with the session.
An WebTransport session over HTTP/3 is considered terminated when either
endpoint closes the stream associated with the CONNECT request that initiated

Choose a reason for hiding this comment

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

  • What happens when one endpoint sends a RESET_STREAM? Is it a protocol violation, or "unclean" session termination?
  • What happens when one endpoint sends a STOP_SENDING on the CONNECT stream?

Copy link
Collaborator Author

@vasilvv vasilvv Sep 8, 2021

Choose a reason for hiding this comment

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

Those are all included in the "closed" term.

Copy link

@afrind afrind Sep 22, 2021

Choose a reason for hiding this comment

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

Should we be more explicit that either peer closes the write half of the stream or you think it's implied?

Copy link
Collaborator Author

@vasilvv vasilvv Oct 13, 2021

Choose a reason for hiding this comment

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

I reworded this to make it explicit that either peer closing it in either way means it's terminated.

@vasilvv
Copy link
Collaborator Author

@vasilvv vasilvv commented Sep 8, 2021

@DavidSchinazi @ekinnear @afrind Updated the PR; please take another look.

Copy link
Collaborator

@DavidSchinazi DavidSchinazi left a comment

Overall looks good modulo removal of the length and some nits

draft-ietf-webtrans-http3.md Show resolved Hide resolved
draft-ietf-webtrans-http3.md Outdated Show resolved Hide resolved
draft-ietf-webtrans-http3.md Outdated Show resolved Hide resolved
draft-ietf-webtrans-http3.md Outdated Show resolved Hide resolved
draft-ietf-webtrans-http3.md Outdated Show resolved Hide resolved
vasilvv and others added 2 commits Sep 17, 2021
Co-authored-by: David Schinazi <DavidSchinazi@users.noreply.github.com>
@vasilvv
Copy link
Collaborator Author

@vasilvv vasilvv commented Sep 21, 2021

I think I addressed all issues; please tell me if I'm missing anything, if not, I'll merge this soon.

cc @DavidSchinazi @ekinnear @afrind

Copy link
Collaborator

@DavidSchinazi DavidSchinazi left a comment

Looks good modulo one small nit

draft-ietf-webtrans-http3.md Outdated Show resolved Hide resolved
Copy link

@ekinnear ekinnear left a comment

Just minor nits, looking good!


: A UTF-8 encoded error message string provided by the application closing the
connection. The message takes up the remainer of the capsule, and its
length MUST NOT exceed 1024 bytes.

Choose a reason for hiding this comment

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

Any particular reason for the choice of length? Just trying to stay under QUIC minimum sizes?

Copy link
Collaborator Author

@vasilvv vasilvv Oct 2, 2021

Choose a reason for hiding this comment

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

Not in particular, just trying to place some bound on the string length, and 1024 seemed like a reasonable number.

Copy link
Collaborator

@DavidSchinazi DavidSchinazi Oct 13, 2021

Choose a reason for hiding this comment

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

Maybe add a sentence explaining the limit?

CLOSE_WEBTRANSPORT_SESSION, the stream MUST be reset with code
H3_MESSAGE_ERROR. The recipient MUST close the stream upon receiving a FIN.
If the sender of CLOSE_WEBTRANSPORT_SESSION does not receive a FIN after some
time, it SHOULD send STOP_SENDING on the CONNECT stream.

Choose a reason for hiding this comment

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

So this isn't an error? Or we think it might be, but we're trying to be more gentle?
Or we expect it to actually happen in real life if there's a high enough RTT?

Copy link

@afrind afrind Sep 22, 2021

Choose a reason for hiding this comment

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

Maybe this bit of text belongs above (around line 369) and be made generic to the case when a peer sends a FIN (without CLOSE_WEBTRANSPORT_SESSION) as well?

Copy link
Collaborator Author

@vasilvv vasilvv Oct 2, 2021

Choose a reason for hiding this comment

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

Well, there isn't really an "error" when it comes to closing things (since you're closing them anyways). But yes, in general, (0, "") is the close code unless specified otherwise.

draft-ietf-webtrans-http3.md Outdated Show resolved Hide resolved
draft-ietf-webtrans-http3.md Outdated Show resolved Hide resolved

: A UTF-8 encoded error message string provided by the application closing the
connection. The message takes up the remainer of the capsule, and its
length MUST NOT exceed 1024 bytes.
Copy link
Collaborator

@DavidSchinazi DavidSchinazi Oct 13, 2021

Choose a reason for hiding this comment

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

Maybe add a sentence explaining the limit?

vasilvv and others added 3 commits Oct 18, 2021
@vasilvv vasilvv merged commit e6bc3bb into ietf-wg-webtrans:main Oct 19, 2021
1 check 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.

5 participants