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: custom priority for push streams #16444

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions lib/internal/http2/core.js
Original file line number Diff line number Diff line change
Expand Up @@ -1804,6 +1804,13 @@ class ServerHttp2Stream extends Http2Stream {

const stream = new ServerHttp2Stream(session, ret, options, headers);

if (options.weight !== undefined) {
stream.priority({
weight: options.weight,
silent: true
});
}

// If the push stream is a head request, close the writable side of
// the stream immediately as there won't be any data sent.
if (headRequest) {
Expand Down
47 changes: 47 additions & 0 deletions test/parallel/test-http2-server-push-priority.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
'use strict';

const common = require('../common');
if (!common.hasCrypto)
common.skip('missing crypto');
const assert = require('assert');
const http2 = require('http2');


const server = http2.createServer();
server.on('stream', common.mustCall((stream, headers, flags) => {
const expectedWeight = 256;
const options = { weight: expectedWeight };
stream.pushStream({}, options, common.mustCall((stream, headers, flags) => {
// The priority of push streams is changed silently by nghttp2,
// without emitting a PRIORITY frame. Flags are ignored.
assert.strictEqual(stream.state.weight, expectedWeight);
assert.strictEqual(flags & http2.constants.NGHTTP2_FLAG_PRIORITY, 0);
}));
stream.respond({
'content-type': 'text/html',
':status': 200
});
stream.end('test');
}));

server.listen(0, common.mustCall(() => {
const port = server.address().port;
const client = http2.connect(`http://localhost:${port}`);

const headers = { ':path': '/' };
const req = client.request(headers);

client.on('stream', common.mustCall((stream, headers, flags) => {
// Since the push priority is set silently, the client is not informed.
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 a priority event that should be emitted, I believe.

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 issue here is that on the wire I wasn't seeing a priority flag on the headers frame (push request), nor a separate priority frame. This is different from client-initiated streams.

Copy link
Member

Choose a reason for hiding this comment

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

oh right right... now I recall... this is actually a bit of a grey area in the current http2 specification: https://tools.ietf.org/html/rfc7540#section-5.3 ... it's actually not clear at all if the server is even allowed to set the priority on a push stream.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From the nghttp2 docs:

nghttp2_submit_push_promise [...]
The flags is currently ignored.

const expectedWeight = http2.constants.NGHTTP2_DEFAULT_WEIGHT;
assert.strictEqual(stream.state.weight, expectedWeight);
assert.strictEqual(flags & http2.constants.NGHTTP2_FLAG_PRIORITY, 0);
}));

req.on('data', common.mustCall(() => {}));
req.on('end', common.mustCall(() => {
server.close();
client.destroy();
}));
req.end();
}));