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

Use quic= instead of v= #235

Merged
merged 5 commits into from
Feb 2, 2017
Merged

Use quic= instead of v= #235

merged 5 commits into from
Feb 2, 2017

Conversation

MikeBishop
Copy link
Contributor

Fixes #229

representation.
On receipt of an Alt-Svc header indicating HTTP/QUIC support, a client MAY
attempt to establish a QUIC connection on the indicated port and, if successful,
send HTTP requests using the mapping described in this document. Servers SHOULD
Copy link
Member

Choose a reason for hiding this comment

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

I think that I would rather the SHOULD here be a MUST. (Then the "but" that follows isn't a "but".)

@LPardue
Copy link
Member

LPardue commented Jan 26, 2017

Section "Registration of Version Hint Alt-Svc Parameter" still refers to "v" rather than "quic".

establishment failure, in which case the client should gracefully fall back to
HTTP/2.
Origins SHOULD list only versions which they support, but MAY omit supported
versions for any reason.
Copy link
Member

Choose a reason for hiding this comment

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

In the host-neutral case the origin is listing versions for the HTTP/QUIC endpoint. I don't think there is a requirement for the advertising host to support any version of QUIC, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The origin has to support those versions at the advertised endpoint, regardless of whether the particular host that sent the header is that endpoint or not.

version "Q034" and version 0x00000001, it would specify the following header:
When multiple versions are supported, the "quic" parameter MAY be repeated
multiple times in a single Alt-Svc entry. For example, if a server supported
both version "Q034" and version 0x00000001, it could specify the following
Copy link
Member

Choose a reason for hiding this comment

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

Do we have enough context to understand what "Q034" is, aside from knowing that an ASCII/UTF-8 encoding of "Q" is 0x51?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's still there primarily because I wanted to be able to show the multiple-entry case. However, it might make more sense to change that to a draft version and leave the ASCII-like versions out entirely.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mm. Except that the draft versions won't be in the final QUIC RFC, so there won't be any context for that reference once it's eventually published. Maybe not.

Copy link
Member

Choose a reason for hiding this comment

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

We could add that context, or we could add a "note to the RFC Editor" to remove Q034 and the related text on publication.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But then we'd still have to give them something to replace it with, and keeping the "how to do multiple" is worthwhile. I've expanded slightly, to refer to it as the version that would be rendered in ASCII as Q034. Given that we allow pretty free-form experimentation in that region, I don't know that we need to elucidate why a server might support an experimental version.

Copy link
Member

@LPardue LPardue left a comment

Choose a reason for hiding this comment

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

LGTM

@martinthomson
Copy link
Member

martinthomson commented Feb 1, 2017

I still have a poor understanding of how @mnot intends to run the process here WRT notify-consensus. I think that given wide notice, discussion at the interim, and absence of any objections, this is probably OK to merge.


Alt-Svc: hq=":443";v="x1";v="cQ034"
Alt-Svc: hq=":443";quic=1;quic=51303334
Copy link
Contributor

Choose a reason for hiding this comment

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

I suspect this is obvious, but it might be nice to explain why 0x00000001 is ASCII encoded as "1" and not "00000001". I assume the idea is that leading '0's may be removed from the hex encoded string?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Basically, yes. I've made that explicit in a9e47f2.

@LPardue
Copy link
Member

LPardue commented Feb 1, 2017 via email

@janaiyengar
Copy link
Contributor

I think we merge the changes to keep our repo and CL stack sane. The chairs can call consensus and if there's no consensus, we can always roll back.

@martinthomson
Copy link
Member

Yep, I'll press the button so that you can blame me when we realize that don't have consensus.

@martinthomson martinthomson merged commit 6b9bbc6 into master Feb 2, 2017
@mnot
Copy link
Member

mnot commented Feb 3, 2017

@martinthomson (too many martins on this repo!) --

notify-consensus is only a bookkeeping measure to remind us to tell the list that we closed an issue.

I think the label you're looking for is editor-ready, and it should only be applied by the chairs. Mike did the right thing by pinging me on the issue, sorry I missed the notification.

This is all amenable to changes, of course, but let's see if we can all get on the same page first.

As a reminder, see also:
https://github.com/quicwg/base-drafts/blob/master/CONTRIBUTING.md#resolving-issues

@MikeBishop MikeBishop deleted the v_quic branch February 7, 2017 21:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants