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

Update WebTransport handshake #251

Closed
wants to merge 1 commit into from
Closed

Update WebTransport handshake #251

wants to merge 1 commit into from

Conversation

rlyshw
Copy link

@rlyshw rlyshw commented Jan 16, 2022

/wt endpoint wasn't working. Needed to modify header negotiation to work again.

Changes mostly come from here:
https://github.com/GoogleChrome/samples/blob/gh-pages/webtransport/webtransport_server.py

@codecov
Copy link

codecov bot commented Feb 1, 2022

Codecov Report

Merging #251 (3fb34f6) into main (9e360a5) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##              main      #251   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           23        23           
  Lines         4572      4572           
=========================================
  Hits          4572      4572           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9e360a5...3fb34f6. Read the comment docs.

@jlaine
Copy link
Contributor

jlaine commented Feb 2, 2022

This doesn't look great, if H3Connection needs to be updated for recent protocol changes let's just change it there, not in the examples. Otherwise anybody wanting to use H3Connection will need to apply these hacks. Also I suspect the client suffers from the same problem.

Also I don't understand the need for H3_DATAGRAM_05, H3_DATAGRAM was already updated in aa8a038

So in essence I think the only thing this PR actually changes is enabling WebSockets?

@jlaine
Copy link
Contributor

jlaine commented Feb 2, 2022

AFAICT most of the changes in this PR are not required. The only addition required for the WebTransport handshake to succeed is adding the sec-webtransport-http3-draft header in the server's response. This is done in 24eca50.

@jlaine jlaine closed this Feb 2, 2022
@jlaine
Copy link
Contributor

jlaine commented Feb 3, 2022

FYI, with the changes in version 0.9.19 (just released) the Google Chrome example can be simplified, I've submitted a PR to that effect in GoogleChrome/samples#764

@yutakahirano

@yutakahirano
Copy link
Contributor

Thank you!

@jlaine
Copy link
Contributor

jlaine commented Feb 4, 2022

You're welcome @yutakahirano, it's in my interest to keep your examples as lean as possible, it reduces the noise on the issue tracker :)

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.

3 participants