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

http2: add altsvc support #17917

Closed
wants to merge 2 commits into from
Closed

http2: add altsvc support #17917

wants to merge 2 commits into from

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Dec 29, 2017

Add support for sending and receiving ALTSVC frames.

As defined by https://tools.ietf.org/html/rfc7838

ping @nodejs/http2

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

http2

@nodejs-github-bot nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Dec 29, 2017
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

LGTM

I would be open to spelling out the name in the API as alternateService or similar

Integer::New(isolate, id),
String::NewFromOneByte(isolate,
altsvc->origin,
v8::NewStringType::kInternalized,
Copy link
Member

Choose a reason for hiding this comment

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

Any reason ehy this is internalized?

Copy link
Member Author

Choose a reason for hiding this comment

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

nah, cut'n'paste error actually

throw new errors.RangeError('ERR_OUT_OF_RANGE', 'stream');

if (origin)
origin = (new URL(origin)).origin;
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 it would be helpful to make sure the URL’s scheme is HTTP or HTTPS (or at least the origin is not ’null’.

Copy link
Member Author

Choose a reason for hiding this comment

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

There is a check for a falsy origin on line 1195.

Copy link
Member

Choose a reason for hiding this comment

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

A “null origin” is not that the returned origin is null but that it is 'null'. Try new URL('abc:').origin, which IMO should be forbidden.

Copy link
Member Author

Choose a reason for hiding this comment

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

ah, right, forgot about that little nuance. Sigh.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, added a check for 'null' in the code and in the test

int32_t id = args[0]->Int32Value(env->context()).ToChecked();

// TODO(jasnell): This is not exactly right as these are supposed to
// be ASCII only.
Copy link
Member

Choose a reason for hiding this comment

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

Is this TODO not currently solvable? We know for a fact that at least the origin is ASCII-only.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, was already working on it :-)

doc/api/http2.md Outdated
* `stream` {number} The numeric identifier of a stream (or `0`). Defaults to
`0`.

Submits an `ALTSVC` frame (as defined by [RFC 7838][]) to the connected client.
Copy link
Member

Choose a reason for hiding this comment

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

Would help if what stream ID 0 (and why it must have an origin) could be explained. Not everyone likes to read RFC’s.

@jasnell
Copy link
Member Author

jasnell commented Dec 30, 2017

@addaleax ... renamed the public API function as suggested.

Copy link
Member

@TimothyGu TimothyGu left a comment

Choose a reason for hiding this comment

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

Mostly nits. Thanks for doing this.

if (len > 16382) // Max length permitted for ALTSVC
throw new errors.TypeError('ERR_HTTP2_ALTSVC_LENGTH');

if (stream === 0 && !origin || origin.length === 0 || origin === 'null')
Copy link
Member

Choose a reason for hiding this comment

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

Maybe the !origin could be written out explicitly as origin !== undefined? (Ditto below.)

And also I think the origin.length check or 'null' check fail it should use a more informative error code.

Copy link
Member

Choose a reason for hiding this comment

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

There is also the issue of if we want to error out on invalid origin when stream is non-zero. I think we should, for the same reason as we are erroring out on non-string type regardless of what stream is. So overall I think the logic should look like

// type checks first
if (origin !== undefined && typeof origin !== 'string') error();

if (origin === undefined) {
  if (stream === 0) error();
} else {
  // we shouldn’t make exceptions for empty origin string either: either it’s undefined or it has to be a valid URL.
  origin = new URL(origin).origin;
  if (origin === 'null') error();
  if (stream !== 0) error();
}

Copy link
Member Author

Choose a reason for hiding this comment

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

If we simplify the signature to allow the second argument to be either a string or a number, then we don't necessarily have to jump through this hoop. Will stew on it a bit more

doc/api/http2.md Outdated
Sending an `ALTSVC` frame with stream ID `0` (the default) indicates that
the alternate service is associated with the given `origin`. Sending an
`ALTSVC` frame with a specific stream ID indicates that the alternate
service is associated with the origin of the given `Http2Stream`.
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, would it be a good idea to make the second parameter number|string, where when it’s number it has to be a valid stream ID and when it’s a string it would be used as the origin (and thus the stream ID is implicitly 0)? That’s only a good idea if we are certain they won’t make it possible to have a stream ID and origin at the same time, but does have the advantage of hiding the 0 as an implementation detail (it’s sort of a magic number otherwise).

Thanks for writing the documentation though: it’s very helpful.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've been stewing over it. I'm starting to have a real dislike for polymorphic function signatures but it would make things a bit easier in both the implementation and on the user.

### Class: ClientHttp2Session
<!-- YAML
added: v8.4.0
-->

#### Event: 'altsvc'
Copy link
Member

Choose a reason for hiding this comment

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

I sort of feel this name should correspond with the name of the method on the server side. I’m ambivalent about whether alternateService is a better name than altsvc though: I feel eventually most people would refer to it as ALTSVC (HTTP/2 frame) or Alt-Svc (HTTP/1.1 header), while technically the name of the header is alternateServices.

Copy link
Member Author

Choose a reason for hiding this comment

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

@addaleax ... given that you were the one that made the original rename suggestion, what do you think here?

Copy link
Member

Choose a reason for hiding this comment

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

@jasnell If everybody here agrees that it’s going to be referred to as ALTSVC anyway, yea, probably best to stick with it?

Either way, yes, +1 for being consistent :)

Copy link
Member Author

Choose a reason for hiding this comment

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

I think altsvc is definitely going to be the most common given the way the header and frame type are defined. I'll switch the API call back :-)


int32_t id = args[0]->Int32Value(env->context()).ToChecked();

// origin and value are both required to be ASCII, handle them as such.
Copy link
Member

Choose a reason for hiding this comment

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

Is this requirement enforced for the value string? If not then we should, to combat the HTTP header encoding debacle that only recently got rectified. In fact IMO we should verify that it only consists of HTTP tokens, similar to the HTTP header checks.

Origin is fine because it’s guaranteed to be ASCII by the URL parser.

Copy link
Member Author

Choose a reason for hiding this comment

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

The value is not limited to token. It will contain a sequence of uri-host constructs (host:port) within quoted strings. At the C++ level, we write these out as one-byte strings so there is at least some level of enforcement there. And on the receiving side, these are treated as one byte strings so the value will be received correctly. Anyone using extended characters will just see those get corrupted. Because of the performance implications and the fact that this is a non-critical API, I don't think we need to validate every character.... although, it would be good to extend the documentation on this a bit more -- as you said, not everyone likes to read RFCs :-)

Copy link
Member

Choose a reason for hiding this comment

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

The additional documentation is always a plus.

Anyone using extended characters will just see those get corrupted.

Well that’s certainly not the best of outcomes. If this is truly a non-critical API, I think we should ensure correctness above performance. Good call on quotation marks though, and in that case I think simply using a /[\x00-\x7f]/ check should be enough.

Copy link
Member

Choose a reason for hiding this comment

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

There may also be some security implications involved, when '\uff20' is treated identically to `'\u0020' by the V8 API.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, added a check

@jasnell
Copy link
Member Author

jasnell commented Dec 30, 2017

@TimothyGu @addaleax ... updated! Went with the two argument signature and reworked the error codes a bit. Also extended the documentation to be more useful. PTAL

@jasnell jasnell mentioned this pull request Dec 30, 2017
11 tasks
@jasnell
Copy link
Member Author

jasnell commented Jan 1, 2018

@@ -32,6 +32,8 @@ const kMaxFrameSize = (2 ** 24) - 1;
const kMaxInt = (2 ** 32) - 1;
const kMaxStreams = (2 ** 31) - 1;

const kQuotedString = /^[\x09\x32\x21-\x5b\x5d-\x7e\x80-\xff]*$/;
Copy link
Member

Choose a reason for hiding this comment

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

Is the x32 meant to be x20?

Copy link
Member

Choose a reason for hiding this comment

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

Also, x80-xff can’t really happen because the specific quoted-string is specified to contain (optionally) a url-host, a colon, and a port number, all of which is in the ASCII range. Removing support for that will make this regex not generic for quoted-string but I think it’s a good change.

Copy link
Member Author

Choose a reason for hiding this comment

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

oh ha!! thinking hex, typed decimal. sigh

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd still prefer to keep the x80-xff range given the definition of quoted-string. Yes, it shouldn't happen, but the spec definition technically allows for it. It will be up to the user to validate that what they get is a valid value.

Copy link
Member

Choose a reason for hiding this comment

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

Okay. I can’t agree with that interpretation of the spec but this is not a blocker for me like the corruption issue was.


The format of the `alt` parameter is strictly defined by [RFC 7838][] as an
ASCII string containing a comma-delimited list of "alternative" protocols
associated with a specific host and port.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe this could mention the clear special value as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, good idea

@jasnell jasnell added the http2 Issues or PRs related to the http2 subsystem. label Jan 2, 2018
Copy link
Member

@TimothyGu TimothyGu left a comment

Choose a reason for hiding this comment

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

LGTM.


if (typeof originOrStream === 'string') {
origin = (new URL(originOrStream)).origin;
if (origin.length === 0 || origin === 'null')
Copy link
Member

Choose a reason for hiding this comment

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

The length check is unnecessary: serialized origins cannot be empty.

doc/api/http2.md Outdated
* `alt` {string} A description of the alternative service configuration as
defined by [RFC 7838][].
* `originOrStream` {string|number} Either a URL string specifying the origin or
the numeric identifier of an active `Http2Stream`.
Copy link
Member

Choose a reason for hiding this comment

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

it is not clear where this number can be found.

let origin;

if (typeof originOrStream === 'string') {
origin = (new URL(originOrStream)).origin;
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 we would be better of in accepting a URL object rather than a string, or an object with an origin  property. The typical usage is to send this down for every session, and passing an object allows a user to avoid parsing a URL for every request.

Copy link
Member Author

Choose a reason for hiding this comment

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

Let's support both. If originOrStream is passed in as an object with an origin property, it will be used. This risks, of course, the value not being strictly ASCII, but we can document that requirement.

Add support for sending and receiving ALTSVC frames.
@jasnell
Copy link
Member Author

jasnell commented Jan 2, 2018

@mcollina ... updated to support passing in URL objects and objects-with-origin. Updated docs and test. PTAL

@jasnell
Copy link
Member Author

jasnell commented Jan 2, 2018

@jasnell
Copy link
Member Author

jasnell commented Jan 3, 2018

CI is good. One failure on freebsd is unrelated.

@jasnell
Copy link
Member Author

jasnell commented Jan 3, 2018

@mcollina ... I plan to get this landed in the morning. Can you take one final look?

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

LGTM

jasnell added a commit that referenced this pull request Jan 3, 2018
Add support for sending and receiving ALTSVC frames.

PR-URL: #17917
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
@jasnell
Copy link
Member Author

jasnell commented Jan 3, 2018

Landed in ce22d6f

@jasnell jasnell closed this Jan 3, 2018
MylesBorins pushed a commit to jasnell/node that referenced this pull request Jan 9, 2018
Add support for sending and receiving ALTSVC frames.

PR-URL: nodejs#17917
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jan 9, 2018
Add support for sending and receiving ALTSVC frames.

Backport-PR-URL: #18050
PR-URL: #17917
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Jan 10, 2018
MylesBorins added a commit that referenced this pull request Jan 10, 2018
Notable change:

* async_hooks:
  - deprecate AsyncHooks Sensitive API and runInAsyncIdScope. Neither
    API were documented. (Andreas Madsen)
    #16972
* deps:
  - update nghttp2 to 1.29.0 (James M Snell)
    #17908
  - upgrade npm to 5.6.0 (Kat Marchán)
    #17535
  - cherry-pick 50f7455 from upstream V8 (Michaël Zasso)
    #16591
* events:
  - remove reaches into _events internals (Anatoli Papirovski)
    #17440
* http:
  - add rawPacket in err of `clientError` event (XadillaX)
    #17672
* http2:
  - implement maxSessionMemory (James M Snell)
    #17967
  - add initial support for originSet (James M Snell)
    #17935
  - add altsvc support (James M Snell)
    #17917
  - perf_hooks integration (James M Snell)
    #17906
* net:
  - remove Socket.prototype.write (Anna Henningsen)
    #17644
  - remove Socket.prototype.listen (Ruben Bridgewater)
    #13735
* repl:
  - show lexically scoped vars in tab completion (Michaël Zasso)
    #16591
* stream:
  - rm {writeable/readable}State.length (Calvin Metcalf)
    #12857
  - add flow and buffer properties to streams (Calvin Metcalf)
    #12855
* util:
  - allow wildcards in NODE_DEBUG variable (Tyler)
    #17609
* zlib:
  - add ArrayBuffer support (Jem Bezooyen)
    #16042
* Addedew collaborator**
  - [starkwang](https://github.com/starkwang) Weijia Wang
* Addedew TSC member**
  - [danbev](https://github.com/danbev) Daniel Bevenius

PR-URL: #18069
MylesBorins pushed a commit that referenced this pull request Jan 10, 2018
Add support for sending and receiving ALTSVC frames.

Backport-PR-URL: #18050
PR-URL: #17917
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
MylesBorins added a commit that referenced this pull request Jan 10, 2018
Notable change:

* async_hooks:
  - deprecate AsyncHooks Sensitive API and runInAsyncIdScope. Neither
    API were documented. (Andreas Madsen)
    #16972
* deps:
  - update nghttp2 to 1.29.0 (James M Snell)
    #17908
  - upgrade npm to 5.6.0 (Kat Marchán)
    #17535
  - cherry-pick 50f7455 from upstream V8 (Michaël Zasso)
    #16591
* events:
  - remove reaches into _events internals (Anatoli Papirovski)
    #17440
* http:
  - add rawPacket in err of `clientError` event (XadillaX)
    #17672
* http2:
  - implement maxSessionMemory (James M Snell)
    #17967
  - add initial support for originSet (James M Snell)
    #17935
  - add altsvc support (James M Snell)
    #17917
  - perf_hooks integration (James M Snell)
    #17906
  - Refactoring and cleanup of Http2Session and Http2Stream destroy
    (James M Snell) #17406
* net:
  - remove Socket.prototype.write (Anna Henningsen)
    #17644
  - remove Socket.prototype.listen (Ruben Bridgewater)
    #13735
* repl:
  - show lexically scoped vars in tab completion (Michaël Zasso)
    #16591
* stream:
  - rm {writeable/readable}State.length (Calvin Metcalf)
    #12857
  - add flow and buffer properties to streams (Calvin Metcalf)
    #12855
* util:
  - allow wildcards in NODE_DEBUG variable (Tyler)
    #17609
* zlib:
  - add ArrayBuffer support (Jem Bezooyen)
    #16042
* Addedew collaborator**
  - [starkwang](https://github.com/starkwang) Weijia Wang
* Addedew TSC member**
  - [danbev](https://github.com/danbev) Daniel Bevenius

PR-URL: #18069
MylesBorins added a commit that referenced this pull request Jan 10, 2018
Notable change:

* async_hooks:
  - deprecate AsyncHooks Sensitive API and runInAsyncIdScope. Neither
    API were documented. (Andreas Madsen)
    #16972
* deps:
  - update nghttp2 to 1.29.0 (James M Snell)
    #17908
  - upgrade npm to 5.6.0 (Kat Marchán)
    #17535
  - cherry-pick 50f7455 from upstream V8 (Michaël Zasso)
    #16591
* events:
  - remove reaches into _events internals (Anatoli Papirovski)
    #17440
* http:
  - add rawPacket in err of `clientError` event (XadillaX)
    #17672
* http2:
  - implement maxSessionMemory (James M Snell)
    #17967
  - add initial support for originSet (James M Snell)
    #17935
  - add altsvc support (James M Snell)
    #17917
  - perf_hooks integration (James M Snell)
    #17906
  - Refactoring and cleanup of Http2Session and Http2Stream destroy
    (James M Snell) #17406
* net:
  - remove Socket.prototype.write (Anna Henningsen)
    #17644
  - remove Socket.prototype.listen (Ruben Bridgewater)
    #13735
* repl:
  - show lexically scoped vars in tab completion (Michaël Zasso)
    #16591
* stream:
  - rm {writeable/readable}State.length (Calvin Metcalf)
    #12857
  - add flow and buffer properties to streams (Calvin Metcalf)
    #12855
* util:
  - allow wildcards in NODE_DEBUG variable (Tyler)
    #17609
* zlib:
  - add ArrayBuffer support (Jem Bezooyen)
    #16042
* Addedew collaborator**
  - [starkwang](https://github.com/starkwang) Weijia Wang
* Addedew TSC member**
  - [danbev](https://github.com/danbev) Daniel Bevenius

PR-URL: #18069
kjin pushed a commit to kjin/node that referenced this pull request May 1, 2018
This commit also includes prerequisite error definitions
from c75f87c and 1698c8e.

Add support for sending and receiving ALTSVC frames.

Backport-PR-URL: nodejs#18050
PR-URL: nodejs#17917
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
MylesBorins pushed a commit that referenced this pull request May 2, 2018
This commit also includes prerequisite error definitions
from c75f87c and 1698c8e.

Add support for sending and receiving ALTSVC frames.

Backport-PR-URL: #18050
Backport-PR-URL: #20456
PR-URL: #17917
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
@MylesBorins MylesBorins mentioned this pull request May 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
http2 Issues or PRs related to the http2 subsystem. lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants