-
Notifications
You must be signed in to change notification settings - Fork 30.2k
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
WIP: http2: improve compatibility with http1, add tests #15633
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
- Loading branch information
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -33,6 +33,7 @@ const { | |
HTTP_STATUS_OK | ||
} = constants; | ||
|
||
let socketWarned = false; | ||
let statusMessageWarned = false; | ||
|
||
// Defines and implements an API compatibility layer on top of the core | ||
|
@@ -71,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(); | ||
|
@@ -212,6 +225,8 @@ class Http2ServerRequest extends Readable { | |
} | ||
|
||
get socket() { | ||
socketWarn(); | ||
|
||
const stream = this[kStream]; | ||
if (stream === undefined) | ||
return; | ||
|
@@ -236,6 +251,9 @@ class Http2ServerRequest extends Readable { | |
} | ||
|
||
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; | ||
} | ||
|
||
|
@@ -313,6 +331,8 @@ class Http2ServerResponse extends Stream { | |
|
||
|
||
get socket() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Similar to statusMessageWarn? We could definitely do that. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yep, exactly like that. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
So getting the remote IP will print a warning? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sounds good. Would that land prior to landing a warning on the There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,47 @@ | ||
// Flags: --expose-http2 | ||
'use strict'; | ||
|
||
const common = require('../common'); | ||
if (!common.hasCrypto) | ||
common.skip('missing crypto'); | ||
const h2 = require('http2'); | ||
|
||
// Http2ServerRequest.socket should warn one time when being accessed | ||
|
||
const unsupportedWarned = common.mustCall(1); | ||
process.on('warning', ({ name, message }) => { | ||
const expectedMessage = | ||
'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.'; | ||
if (name === 'UnsupportedWarning' && message === expectedMessage) | ||
unsupportedWarned(); | ||
}); | ||
|
||
const server = h2.createServer(); | ||
server.listen(0, common.mustCall(function() { | ||
const port = server.address().port; | ||
server.once('request', common.mustCall(function(request, response) { | ||
request.socket; | ||
request.socket; // should not warn | ||
response.socket; // should not warn | ||
response.end(); | ||
})); | ||
|
||
const url = `http://localhost:${port}`; | ||
const client = h2.connect(url, common.mustCall(function() { | ||
const headers = { | ||
':path': '/', | ||
':method': 'GET', | ||
':scheme': 'http', | ||
':authority': `localhost:${port}` | ||
}; | ||
const request = client.request(headers); | ||
request.on('end', common.mustCall(function() { | ||
client.destroy(); | ||
server.close(); | ||
})); | ||
request.end(); | ||
request.resume(); | ||
})); | ||
})); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,47 @@ | ||
// Flags: --expose-http2 | ||
'use strict'; | ||
|
||
const common = require('../common'); | ||
if (!common.hasCrypto) | ||
common.skip('missing crypto'); | ||
const h2 = require('http2'); | ||
|
||
// Http2ServerResponse.socket should warn one time when being accessed | ||
|
||
const unsupportedWarned = common.mustCall(1); | ||
process.on('warning', ({ name, message }) => { | ||
const expectedMessage = | ||
'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.'; | ||
if (name === 'UnsupportedWarning' && message === expectedMessage) | ||
unsupportedWarned(); | ||
}); | ||
|
||
const server = h2.createServer(); | ||
server.listen(0, common.mustCall(function() { | ||
const port = server.address().port; | ||
server.once('request', common.mustCall(function(request, response) { | ||
response.socket; | ||
response.socket; // should not warn | ||
request.socket; // should not warn | ||
response.end(); | ||
})); | ||
|
||
const url = `http://localhost:${port}`; | ||
const client = h2.connect(url, common.mustCall(function() { | ||
const headers = { | ||
':path': '/', | ||
':method': 'GET', | ||
':scheme': 'http', | ||
':authority': `localhost:${port}` | ||
}; | ||
const request = client.request(headers); | ||
request.on('end', common.mustCall(function() { | ||
client.destroy(); | ||
server.close(); | ||
})); | ||
request.end(); | ||
request.resume(); | ||
})); | ||
})); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this include a basic type check to ensure that
method
is a non-empty string?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. http1 obviously didn't have it since it's not a getter/setter but I don't see why we couldn't do that for http2.