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.

Sign up for GitHub

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

Jump to bottom

[WebTransport] Add support for multiples capsules #30982

Merged
merged 1 commit into from Oct 1, 2021
Merged

[WebTransport] Add support for multiples capsules #30982

merged 1 commit into from Oct 1, 2021

Conversation

yutakahirano
Copy link
Contributor

Add support for multiple capsules on the session stream.

@yutakahirano
Copy link
Contributor Author

Hi @vasilvv, can you take a look at this change?

This is on top of #30945, so you can see the diff here.

bashi
bashi reviewed Sep 27, 2021
View reviewed changes
Copy link
Member

@bashi bashi left a comment

Choose a reason for hiding this comment

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

Defer to @vasilvv's review for details, just minor comments.

tools/webtransport/h3/webtransport_h3_server.py Outdated
Represents the Capsule concept defined in
https://ietf-wg-masque.github.io/draft-ietf-masque-h3-datagram/draft-ietf-masque-h3-datagram.html#name-capsules.
"""
def __init__(self, type: int, data: bytes) -> None:
Copy link
Member

Choose a reason for hiding this comment

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

Why can't this be CapsuleType?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because we may have a capsule with an unknown type.

tools/webtransport/h3/webtransport_h3_server.py Outdated Show resolved Hide resolved
vasilvv
vasilvv reviewed Sep 28, 2021
View reviewed changes
tools/webtransport/h3/webtransport_h3_server.py Outdated
# https://www.ietf.org/archive/id/draft-ietf-masque-h3-datagram-03.html.
DATAGRAM = 0xff37a0
REGISTER_DATAGRAM_CONTEXT = 0xff37a1
REGISTER_DATAGRAM_NO_CONTEXT = 0xff37a2
Copy link
Contributor

Choose a reason for hiding this comment

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

As a note, h3-datagram-draft04 actually requires us to receive at least REGISTER_DATAGRAM_NO_CONTEXT (we should also support DATAGRAM).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we should also support DATAGRAM

For WebTransport over HTTP/3 we use https://ietf-wg-masque.github.io/draft-ietf-masque-h3-datagram/draft-ietf-masque-h3-datagram.html#name-http-3-datagram-format rather than https://ietf-wg-masque.github.io/draft-ietf-masque-h3-datagram/draft-ietf-masque-h3-datagram.html#name-the-datagram-capsule, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

Chromium accepts both, though I don't believe we send the DATAGRAM capsule currently.

tools/webtransport/h3/webtransport_h3_server.py Outdated
self._type = self._buffer.pull_uint_var()
if self._length is None:
self._length = self._buffer.pull_uint_var()
if self._buffer.capacity - self._buffer.tell() < self._length:
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this line actually required? I'd assume the line below would just throw if that condition is not met.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, This is needed as the exception can be rethrown in the except block.

tools/webtransport/h3/webtransport_h3_server.py Show resolved Hide resolved
tools/webtransport/h3/webtransport_h3_server.py Show resolved Hide resolved
bashi
bashi reviewed Sep 28, 2021
View reviewed changes
tools/serve/serve.py Outdated Show resolved Hide resolved
tools/webtransport/h3/capsule.py Show resolved Hide resolved
tools/webtransport/h3/test_capsule.py Show resolved Hide resolved
@yutakahirano
Copy link
Contributor Author

@vasilvv can you take a look again?

bashi
bashi approved these changes Sep 30, 2021
View reviewed changes
Copy link
Member

@bashi bashi left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

tools/webtransport/h3/capsule.py Outdated Show resolved Hide resolved
tools/webtransport/h3/capsule.py Show resolved Hide resolved
@yutakahirano yutakahirano mentioned this pull request Oct 1, 2021
Merged
@yutakahirano @bashi
[WebTransport] Add support for more capsules
51546cf 
Co-authored-by: bashi <bashi@chromium.org>
@yutakahirano yutakahirano merged commit e4d5edb into master Oct 1, 2021
@yutakahirano yutakahirano deleted the yhirano/more-capsules branch October 1, 2021 10:59
Gabisampaio pushed a commit to Gabisampaio/wpt that referenced this pull request Nov 18, 2021
@yutakahirano @bashi @Gabisampaio
[WebTransport] Add support for more capsules (web-platform-tests#30982)
4178808 
Co-authored-by: bashi <bashi@chromium.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bashi bashi bashi approved these changes

@vasilvv vasilvv vasilvv approved these changes

@foolip foolip Awaiting requested review from foolip

@jgraham jgraham Awaiting requested review from jgraham

Assignees

@foolip foolip

Labels
infra serve webtransport wg-webtransport
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@yutakahirano @bashi @vasilvv @foolip @wpt-pr-bot