Skip to content

WIP: http2: improve compatibility with http1, add tests #15633

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

Closed
apapirovski wants to merge 2 commits into from
Closed

WIP: http2: improve compatibility with http1, add tests #15633

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2e32a51
http2: improve compatibility with http1, add tests
apapirovski Sep 27, 2017
680e507
squash: add method setter validation & warn on accessing socket
apapirovski Sep 28, 2017
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
197 changes: 136 additions & 61 deletions lib/internal/http2/compat.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ const kHeaders = Symbol('headers');
const kRawHeaders = Symbol('rawHeaders');
const kTrailers = Symbol('trailers');
const kRawTrailers = Symbol('rawTrailers');
const kSetHeader = Symbol('setHeader');

const {
NGHTTP2_NO_ERROR,
Expand All @@ -32,14 +33,15 @@ const {
HTTP_STATUS_OK
} = constants;

let socketWarned = false;
let statusMessageWarned = false;

// Defines and implements an API compatibility layer on top of the core
// HTTP/2 implementation, intended to provide an interface that is as
// close as possible to the current require('http') API

function assertValidHeader(name, value) {
if (name === '')
if (name === '' || typeof name !== 'string')
throw new errors.TypeError('ERR_INVALID_HTTP_TOKEN', 'Header name', name);
if (isPseudoHeader(name))
throw new errors.Error('ERR_HTTP2_PSEUDOHEADER_NOT_ALLOWED');
Expand Down Expand Up @@ -70,6 +72,18 @@ function statusMessageWarn() {
}
}

function socketWarn() {
if (socketWarned === false) {
process.emitWarning(
'Because the of the specific serialization and processing requirements ' +
'imposed by the HTTP/2 protocol, it is not recommended for user code ' +
'to read data from or write data to a Socket instance.',
'UnsupportedWarning'
);
socketWarned = true;
}
}

function onStreamData(chunk) {
if (!this[kRequest].push(chunk))
this.pause();
Expand Down Expand Up @@ -141,6 +155,10 @@ function resumeStream(stream) {
stream.resume();
}

function unsetStream(self) {
self[kStream] = undefined;
}

class Http2ServerRequest extends Readable {
constructor(stream, headers, options, rawHeaders) {
super(options);
Expand Down Expand Up @@ -170,12 +188,8 @@ class Http2ServerRequest extends Readable {
this.on('resume', onRequestResume);
}

get closed() {
return this[kState].closed;
}

get code() {
return this[kState].closedCode;
get complete() {
return this._readableState.ended || this[kState].closed;
}

get stream() {
Expand Down Expand Up @@ -211,6 +225,8 @@ class Http2ServerRequest extends Readable {
}

get socket() {
socketWarn();

const stream = this[kStream];
if (stream === undefined)
return;
Expand All @@ -234,6 +250,13 @@ class Http2ServerRequest extends Readable {
return this[kHeaders][HTTP2_HEADER_METHOD];
}

set method(method) {
if (typeof method !== 'string' || method.trim() === '')
throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'method', 'string');

this[kHeaders][HTTP2_HEADER_METHOD] = method;
}

get authority() {
return this[kHeaders][HTTP2_HEADER_AUTHORITY];
}
Expand Down Expand Up @@ -264,18 +287,20 @@ class Http2ServerRequest extends Readable {
state.closedCode = Number(code);
state.closed = true;
this.push(null);
process.nextTick(() => (this[kStream] = undefined));
process.nextTick(unsetStream, this);
}
}

class Http2ServerResponse extends Stream {
constructor(stream, options) {
super(options);
this[kState] = {
closed: false,
closedCode: NGHTTP2_NO_ERROR,
ending: false,
headRequest: false,
sendDate: true,
statusCode: HTTP_STATUS_OK,
closed: false,
closedCode: NGHTTP2_NO_ERROR
};
this[kHeaders] = Object.create(null);
this[kTrailers] = Object.create(null);
Expand All @@ -290,17 +315,32 @@ class Http2ServerResponse extends Stream {
stream.on('finish', onfinish);
}

// User land modules such as finalhandler just check truthiness of this
// but if someone is actually trying to use this for more than that
// then we simply can't support such use cases
get _header() {
return this.headersSent;
}

get finished() {
const stream = this[kStream];
return stream === undefined || stream._writableState.ended;
return stream === undefined ||
stream._writableState.ended ||
this[kState].closed;
}

get closed() {
return this[kState].closed;

get socket() {
Copy link
Member

@jasnell jasnell Sep 28, 2017

Choose a reason for hiding this comment

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

I'm wondering if we shouldn't more proactively warn people away from accessing this... e.g. by using an explicit runtime deprecation warning right from the start.

Copy link
Contributor Author

@apapirovski apapirovski Sep 28, 2017

Choose a reason for hiding this comment

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

Similar to statusMessageWarn? We could definitely do that.

Copy link
Member

@jasnell jasnell Sep 28, 2017

Choose a reason for hiding this comment

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

yep, exactly like that.

Copy link
Member

@dougwilson dougwilson Sep 28, 2017

Choose a reason for hiding this comment

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

As long as there is a way to retrieve the remote IP address for the socket (and local port, etc.); ideally symmetrically with http/1.

Copy link
Contributor Author

@apapirovski apapirovski Sep 28, 2017

Choose a reason for hiding this comment

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

I think we will keep the socket available but just add a one-time warning re: its usage.

Copy link
Member

@jasnell jasnell Sep 28, 2017

Choose a reason for hiding this comment

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

Good point and a good reminder! I had that marked down quite some time ago as a todo for an API that would make fetching those details easier but hadn't gotten around to it. We can make that data available via the Http2Session object so that the socket property does not have to be accessed.

Copy link
Member

@dougwilson dougwilson Sep 28, 2017

Choose a reason for hiding this comment

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

I think we will keep the socket available but just add a one-time warning re: its usage.

So getting the remote IP will print a warning?

Copy link
Member

@jasnell jasnell Sep 28, 2017

Choose a reason for hiding this comment

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

just getting it via the request and response objects should print a warning. We should add apis that allow this information to be retrieved independently of that so that there's a path that (a) does not print the warning and (b) does not mean users are accessing the socket directly.

Copy link
Member

@dougwilson dougwilson Sep 28, 2017

Choose a reason for hiding this comment

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

Sounds good. Would that land prior to landing a warning on the socket getter, then?

Copy link
Member

@jasnell jasnell Sep 28, 2017

Choose a reason for hiding this comment

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

We could probably do those at the same time actually.

socketWarn();

const stream = this[kStream];
if (stream === undefined)
return;
return stream.session.socket;
}

get code() {
return this[kState].closedCode;
get connection() {
return this.socket;
}

get stream() {
Expand All @@ -309,7 +349,7 @@ class Http2ServerResponse extends Stream {

get headersSent() {
const stream = this[kStream];
return stream !== undefined ? stream.headersSent : this[kState].headersSent;
return stream === undefined ? this[kState].headersSent : stream.headersSent;
}

get sendDate() {
Expand Down Expand Up @@ -339,7 +379,7 @@ class Http2ServerResponse extends Stream {

name = name.trim().toLowerCase();
assertValidHeader(name, value);
this[kTrailers][name] = String(value);
this[kTrailers][name] = value;
}

addTrailers(headers) {
Expand Down Expand Up @@ -379,6 +419,13 @@ class Http2ServerResponse extends Stream {
if (typeof name !== 'string')
throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'name', 'string');

const stream = this[kStream];
const state = this[kState];
if (!stream && !state.headersSent)
return;
if (state.headersSent || stream.headersSent)
throw new errors.Error('ERR_HTTP2_HEADERS_SENT');

name = name.trim().toLowerCase();
delete this[kHeaders][name];
}
Expand All @@ -387,9 +434,20 @@ class Http2ServerResponse extends Stream {
if (typeof name !== 'string')
throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'name', 'string');

const stream = this[kStream];
const state = this[kState];
if (!stream && !state.headersSent)
return;
if (state.headersSent || stream.headersSent)
throw new errors.Error('ERR_HTTP2_HEADERS_SENT');

this[kSetHeader](name, value);
}

[kSetHeader](name, value) {
name = name.trim().toLowerCase();
assertValidHeader(name, value);
this[kHeaders][name] = String(value);
this[kHeaders][name] = value;
}

get statusMessage() {
Expand All @@ -403,12 +461,22 @@ class Http2ServerResponse extends Stream {
}

flushHeaders() {
const stream = this[kStream];
if (stream !== undefined && stream.headersSent === false)
this[kBeginSend]();
const state = this[kState];
if (!state.closed && !this[kStream].headersSent) {
this.writeHead(state.statusCode);
}
}

writeHead(statusCode, statusMessage, headers) {
const state = this[kState];

if (state.closed) {
throw new errors.Error('ERR_HTTP2_STREAM_CLOSED');
}
if (this[kStream].headersSent) {
throw new errors.Error('ERR_HTTP2_HEADERS_SENT');
}

if (typeof statusMessage === 'string') {
statusMessageWarn();
}
Expand All @@ -417,24 +485,16 @@ class Http2ServerResponse extends Stream {
headers = statusMessage;
}

const stream = this[kStream];
if (stream === undefined) {
throw new errors.Error('ERR_HTTP2_STREAM_CLOSED');
}
if (stream.headersSent === true) {
throw new errors.Error('ERR_HTTP2_INFO_HEADERS_AFTER_RESPOND');
}

if (typeof headers === 'object') {
const keys = Object.keys(headers);
let key = '';
for (var i = 0; i < keys.length; i++) {
key = keys[i];
this.setHeader(key, headers[key]);
this[kSetHeader](key, headers[key]);
}
}

this.statusCode = statusCode;
state.statusCode = statusCode;
this[kBeginSend]();
}

Expand All @@ -446,20 +506,28 @@ class Http2ServerResponse extends Stream {
encoding = 'utf8';
}

if (stream === undefined) {
if (this[kState].closed) {
const err = new errors.Error('ERR_HTTP2_STREAM_CLOSED');
if (typeof cb === 'function')
process.nextTick(cb, err);
else
throw err;
return;
}
this[kBeginSend]();
if (!stream.headersSent) {
this.writeHead(this[kState].statusCode);
}
return stream.write(chunk, encoding, cb);
}

end(chunk, encoding, cb) {
const stream = this[kStream];
const state = this[kState];

if ((state.closed || state.ending) &&
(stream === undefined || state.headRequest === stream.headRequest)) {
return false;
}

if (typeof chunk === 'function') {
cb = chunk;
Expand All @@ -468,19 +536,31 @@ class Http2ServerResponse extends Stream {
cb = encoding;
encoding = 'utf8';
}
if (this.finished === true) {
return false;
}

if (chunk !== null && chunk !== undefined) {
this.write(chunk, encoding);
}

const isFinished = this.finished;
state.headRequest = stream.headRequest;
state.ending = true;

if (typeof cb === 'function') {
stream.once('finish', cb);
if (isFinished)
this.once('finish', cb);
else
stream.once('finish', cb);
}

if (!stream.headersSent) {
this.writeHead(this[kState].statusCode);
}

this[kBeginSend]({ endStream: true });
stream.end();
if (isFinished) {
this[kFinish]();
} else {
stream.end();
}
}

destroy(err) {
Expand All @@ -500,7 +580,7 @@ class Http2ServerResponse extends Stream {
if (typeof callback !== 'function')
throw new errors.TypeError('ERR_INVALID_CALLBACK');
const stream = this[kStream];
if (stream === undefined) {
if (this[kState].closed) {
process.nextTick(callback, new errors.Error('ERR_HTTP2_STREAM_CLOSED'));
return;
}
Expand All @@ -510,43 +590,38 @@ class Http2ServerResponse extends Stream {
});
}

[kBeginSend](options) {
const stream = this[kStream];
if (stream !== undefined &&
stream.destroyed === false &&
stream.headersSent === false) {
const headers = this[kHeaders];
headers[HTTP2_HEADER_STATUS] = this[kState].statusCode;
options = options || Object.create(null);
options.getTrailers = (trailers) => {
Object.assign(trailers, this[kTrailers]);
};
stream.respond(headers, options);
}
[kBeginSend]() {
const state = this[kState];
const headers = this[kHeaders];
headers[HTTP2_HEADER_STATUS] = state.statusCode;
const options = {
endStream: state.ending,
getTrailers: (trailers) => Object.assign(trailers, this[kTrailers])
};
this[kStream].respond(headers, options);
}

[kFinish](code) {
const stream = this[kStream];
const state = this[kState];
if (state.closed)
if (state.closed || stream.headRequest !== state.headRequest) {
return;
}
if (code !== undefined)
state.closedCode = Number(code);
state.closed = true;
state.headersSent = this[kStream].headersSent;
this.end();
process.nextTick(() => (this[kStream] = undefined));
state.headersSent = stream.headersSent;
process.nextTick(unsetStream, this);
this.emit('finish');
}

// TODO doesn't support callbacks
writeContinue() {
const stream = this[kStream];
if (stream === undefined ||
stream.headersSent === true ||
stream.destroyed === true) {
if (this[kState].closed || stream.headersSent) {
return false;
}
this[kStream].additionalHeaders({
stream.additionalHeaders({
[HTTP2_HEADER_STATUS]: HTTP_STATUS_CONTINUE
});
return true;
Expand Down
Loading