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

Process 100, 102-199 status codes according to specs. #18033

Closed
wants to merge 20 commits into
base: master
from

Conversation

@miles-po
Contributor

miles-po commented Jan 8, 2018

Relevant references:
RFC7231 Section 6.2.1
RFC2518
RFC8297

This affects several modules downstream that use the http
module, e.g., node-fetch, all of whom violate HTTP RFCs
due to this module. As such, this could introduce a
breaking change for downstream if HTTP standards were
ignored in an ad-hoc fashion.

101 Upgrade is excluded due to its non-informational
processing according to RFC7231, Section 6.2.2.

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)

http, http2, https

@miles-po

This comment has been minimized.

Contributor

miles-po commented Jan 8, 2018

I did not include documentation as I believe the RFCs cover this adequately, and the aim was to match the specs where Node was currently non-conforming.

I admit that the tests are a bit opaque to me. I tried to find a good template for 100 Continue behavior, but it was unclear whether server or client was being tested or where the status code was being provided. Any help in this area is appreciated.

@jasnell

This comment has been minimized.

Member

jasnell commented Jan 8, 2018

Generally +1 on this but will need to give it a more thorough review. I'm wondering if this has to be semver-major tho as I can definitely see the possibility of this breaking at least someone.

Ping @nodejs/http and @nodejs/tsc, please take a look

@TimothyGu

+1 on this change, but it needs a test.

I'd say semver-major is appropriate for this, given people may be depending on the existing (broken) behavior.

socket.removeListener('data', socketOnData);
socket.removeListener('end', socketOnEnd);
freeParser(parser, req, socket);
}
}
// client
function informational(status) {

This comment has been minimized.

@TimothyGu

TimothyGu Jan 8, 2018

Member

nit: name this statusIsInformational to be a bit more explicit.

This comment has been minimized.

@miles-po

miles-po Jan 8, 2018

Contributor

Done

@mcollina

This comment has been minimized.

Member

mcollina commented Jan 8, 2018

👍 for semver-major.

I'm fine with the change itself, but it needs tests.

@miles-po

This comment has been minimized.

Contributor

miles-po commented Jan 8, 2018

@TimothyGu As mentioned earlier, I hit a stumbling block with regard to tests. Node's test setup is not familiar to me and the test code for 100s that I'd use as a template is similarly opaque. I'll help any way I can, but I'm blocked by my lack of familiarity at the moment.

@jasnell jasnell added the semver-major label Jan 8, 2018

@BridgeAR

This comment has been minimized.

Member

BridgeAR commented Feb 2, 2018

@miles-po you can run the tests with make -j4 test (check the Makefile for different possible tasks). It is also possible to run tests individually by e.g. python tools/test.py test/parallel/test-http-YOUR_TEST_NAME.js. If you use Windows, please check the vcbuild.bat.

You can write the test just the same as you would write code anywhere else that you would expect to succeed in a specific way. The only differences is that you have to include require('../common'); as first statement and that you have access to more Node.js internal helper functions (e.g. the ones found in common).

@Fishrock123

This comment has been minimized.

Member

Fishrock123 commented Feb 2, 2018

I think it is about time we plan to do this in a near-term major version.

@addaleax

This comment has been minimized.

Member

addaleax commented Feb 2, 2018

@Fishrock123 What are you saying? That this PR should go into 10.0.0?

@addaleax addaleax added this to the 10.0.0 milestone Feb 2, 2018

// to the content-length of the entity-body had the request
// been a GET.
var isHeadResponse = req.method === 'HEAD';
debug('AGENT isHeadResponse', isHeadResponse);

This comment has been minimized.

@bnoordhuis

bnoordhuis Feb 3, 2018

Member

I removed this a while ago, see #17806 (comment). Can you do the same?

This comment has been minimized.

@miles-po

miles-po Feb 4, 2018

Contributor

Done.

This comment has been minimized.

@BridgeAR

BridgeAR Feb 12, 2018

Member

It seems like this got back in / was not removed?

debug('AGENT isHeadResponse', isHeadResponse);
if (statusIsInformational(res.statusCode)) {
// restart the parser, as this is a 1xx informational message.

This comment has been minimized.

@bnoordhuis

bnoordhuis Feb 3, 2018

Member

Nit: please capitalize comments.

This comment has been minimized.

@miles-po

miles-po Feb 4, 2018

Contributor

Done.

@miles-po

This comment has been minimized.

Contributor

miles-po commented Feb 4, 2018

Added test for 102 Processing. Allows for multiple 1xx responses. Updated docs.

Added a new method to ServerResponse: writeProcessing. This is similar to writeContinue. Had considered making a more generic writeInformation to handle all cases, but 102 does not have headers (like 100) while the currently experimental 103 Extra Hints is useless without headers.

The continue event is specific to 100 Continue and I didn't think should be reused for other information statuses. I created the information event to be triggered for any 1xx status. The information event passes an object with statusCode to distinguish what triggered it. This could be used for 100 Continue as well. When 103 is eventually handled explicitly, passing header information or other relevant information should be straightforward.

Open questions I had about 103 were whether to allow headers to be passed to it like writeHead and whether headers would be shared implicitly through to the main response. These questions however are out of scope for this particular PR but may affect the API.

As this is semver-major, we're in a good time to allow for discussion on the API without harsh deadlines. Suggestions welcome.

@mcollina

The title of this PR is about "102-199" status code, but I see code to handle 102 only.

Regarding 103, I would emit the headers as part of the event. I would not mesh them with the proper response header.

I'm not really fond of the 'information' name, but I don't want to bikeshed it either.

// NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM,
// DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE
// USE OR OTHER DEALINGS IN THE SOFTWARE.

This comment has been minimized.

@mcollina

mcollina Feb 5, 2018

Member

The header is not needed.

This comment has been minimized.

@miles-po

miles-po Feb 5, 2018

Contributor

Done.

function _writeInformation(statusCode, statusMessage, cb) {
this._writeRaw(`HTTP/1.1 ${statusCode} ${statusMessage}${CRLF}${CRLF}`,
'ascii', cb);
}

This comment has been minimized.

@mcollina

mcollina Feb 5, 2018

Member

can you make writeInformation as a top level function? There is no need to attach it to the prototype.
Another option is to just call _writeRaw in both cases, I do not see much harm in that duplication.

This comment has been minimized.

@miles-po

miles-po Feb 5, 2018

Contributor

Done.

assert.strictEqual(body, test_res_body, 'Response body doesn\'t match.');
assert.ok('abcd' in res.headers, 'Response headers missing.');
server.close();
process.exit();

This comment has been minimized.

@mcollina

mcollina Feb 5, 2018

Member

is this process.exit()  needed? Can we remove it?

This comment has been minimized.

@miles-po

miles-po Feb 5, 2018

Contributor

Done.

@@ -400,6 +400,14 @@ Emitted when the server sends a '100 Continue' HTTP response, usually because
the request contained 'Expect: 100-continue'. This is an instruction that
the client should send the request body.
### Event: 'information'
<!-- YAML
added: v10.0.0

This comment has been minimized.

@mcollina

mcollina Feb 5, 2018

Member

This should be REPLACEME. It will be replaced upon release.

This comment has been minimized.

@miles-po

miles-po Feb 5, 2018

Contributor

Done.

@miles-po

This comment has been minimized.

Contributor

miles-po commented Feb 5, 2018

The title of the PR is accurate. While only 102 is supported explicitly in the server, the client now allows for any informational message to be passed without ignoring the final response, which was the original intention of this patch. Unfortunately, there was no way to actually test this without added some basic 102 functionality to the server.

I had considered adding 103 support, but as that RFC is still on the "Experimental" track, I decided against it. I guess that was my original thought behind adding that new method, _writeInformation. That it would serve as a launching point for further work in that area. But as you said, it just adds noise to the prototype, so I'm happy to remove it again.

Since 104-199 are not yet defined at all, I thought it best to have the client treat it like one would choose any informational message (allowing for folks with non-standard usage of these codes), but does not affect the client API at all. On the server side, this was not possible. An event can be passed, but how to actually send the headers is not well defined in the RFC.

Should all headers defined so far be sent, or just a user-defined subset? When the headers for the 103 are sent, should those be passed on to next response by default or should this be explicit? What about second or third 103 responses? Should they resend all defined headers or just the changed ones? Should two or three (or ten) 103 messages be allowed? Should there be a limit?

Rather than make a blind stab on this experimental RFC, I punted for now and just concentrated on the specs I could verify.

Comments/suggestions welcome of course.

@mcollina

LGTM

Emitted when the server sends a 1xx response (excluding 101 Upgrade). This
event is emitted with a object, which can be used to retrieve the status code.

This comment has been minimized.

@jasnell

jasnell Feb 5, 2018

Member

This change should also be added to the http2 compat API

This comment has been minimized.

@jasnell

jasnell Feb 5, 2018

Member

Should also explain why 101 Upgrade is not included in this

This comment has been minimized.

@mcollina

mcollina Feb 5, 2018

Member

Yeah, good spot.

This comment has been minimized.

@jasnell

jasnell Feb 5, 2018

Member

Also an example illustrating what "This event is emitted with a object" means would be helpful.

This comment has been minimized.

@miles-po

miles-po Feb 10, 2018

Contributor

Quick question: Should 101s actually fire the event too?

I excluded 101 Upgrade since this was typically used to make TLS upgrades (rare) or Web Socket connections (typical). I don't work on the Socket.IO project—for example—so I don't know the potential drawbacks. I suppose performance wouldn't suffer measurably just from firing an event with no listeners.

This comment has been minimized.

@miles-po

miles-po Feb 10, 2018

Contributor

I take that back. 101 Upgrade messages are handled so differently, they can break the request/response model dramatically. Web Sockets being a prime example but TLS as well. Leaving as is.

@mcollina

This comment has been minimized.

@BridgeAR

This comment has been minimized.

Member

BridgeAR commented Feb 6, 2018

@BridgeAR

This comment has been minimized.

Member

BridgeAR commented Feb 9, 2018

@miles-po would you be so kind and rebase this PR and force-push-with-lease afterwards? Otherwise it is not possible to start the CI.

miles-po added some commits Jan 8, 2018

Process 100, 102-199 status codes according to specs.
Relevant references:
 RFC7231 Section 6.2.1
 RFC2518
 RFC8297

This affects several modules downstream that use the http
module, e.g., node-fetch, all of whom violate HTTP RFCs
due to this module. As such, this could introduce a
breaking change for downstream if HTTP standards were
ignored in an ad-hoc fashion.

101 Upgrade is excluded due to its non-informational
processing according to RFC7231, Section 6.2.2.
Firing event for information messages
Must be distinct from `continue` event due to slightly different
behavior/triggering rules from 102/103 status codes. For example,
103 is explicitly made to take headers for pre-processing while 100
status codes never include headers.
Adding writeInformation to handle 1xx status codes
`writeContinue` is highly specific to 100 status codes and sets state
for further processing. For example, keep-alive behavior is changed by
100 responses. `writeContinue` also cannot set header values, needed by
103 responses (so far).

@BridgeAR BridgeAR removed the author ready label Feb 12, 2018

@miles-po

This comment has been minimized.

Contributor

miles-po commented Feb 12, 2018

Not clear to me where those changes for http2 are to be made. I had assumed (obviously incorrectly) that http2 followed the underscore-prefixed functionality. Could someone narrow down the search please? A line number would suffice.

@mcollina

This comment has been minimized.

Member

mcollina commented Feb 12, 2018

Most of the http2 code lives in:

https://github.com/nodejs/node/blob/master/lib/internal/http2/core.js
https://github.com/nodejs/node/blob/master/lib/internal/http2/compat.js

I think you should add the functionality to https://github.com/nodejs/node/blob/master/lib/internal/http2/core.js#L1447 (and descending classes, the Server and Client pairs). Also the compat API: https://github.com/nodejs/node/blob/master/lib/internal/http2/compat.js#L212 which will need to surface the information as well.

@TimothyGu

This comment has been minimized.

Member

TimothyGu commented Feb 12, 2018

I'm okay with separating the HTTP/2 things into a new PR or commit. This will allow easier backporting to releases that do not have the http2 module.

@mcollina

This comment has been minimized.

Member

mcollina commented Feb 12, 2018

👍to two PRs. They can also be two separate commits.
If that's the approach, the docs for http2 should not be changed in this PR.

Removed HTTP/2 doc updates
HTTP/2 changes will be done in a separate PR.
socket.removeListener('data', socketOnData);
socket.removeListener('end', socketOnEnd);
freeParser(parser, req, socket);
}
}
// client

This comment has been minimized.

@BridgeAR

BridgeAR Feb 12, 2018

Member

I am not sure what this comment stands for.

This comment has been minimized.

@miles-po

miles-po Feb 12, 2018

Contributor

I do not either. I was following what looked like existing convention in this source file. Removing.

// to the content-length of the entity-body had the request
// been a GET.
var isHeadResponse = req.method === 'HEAD';
debug('AGENT isHeadResponse', isHeadResponse);

This comment has been minimized.

@BridgeAR

BridgeAR Feb 12, 2018

Member

It seems like this got back in / was not removed?

res.on('end', function() {
console.error('Got full response.');
assert.strictEqual(body, test_res_body, 'Response body doesn\'t match.');
assert.ok('abcd' in res.headers, 'Response headers missing.');

This comment has been minimized.

@BridgeAR

BridgeAR Feb 12, 2018

Member

Leaving out the error message is actually better because in case of an error it would say:
The expression assert.ok('abcd' in res.headers) evaluated to a falsy value.

res.on('data', function(chunk) { body += chunk; });
res.on('end', function() {
console.error('Got full response.');
assert.strictEqual(body, test_res_body, 'Response body doesn\'t match.');

This comment has been minimized.

@BridgeAR

BridgeAR Feb 12, 2018

Member

Not having any message will probably be even better because it will show what values got in.

});
req.on('response', function(res) {
assert.strictEqual(processing_count, 2,

This comment has been minimized.

@BridgeAR

BridgeAR Feb 12, 2018

Member

This could use common/countdown.js. That would be more intuitive.

path: '/length_request'
};
// make a request

This comment has been minimized.

@BridgeAR

BridgeAR Feb 12, 2018

Member

Non-blocking nit: Using capital letters as first character would be great :-) And maybe also a dot at the end.

Fixed rebase errors and nits
* While rebasing, old content resurfaced. Re-removed.
* Removed errant 'client' comment
* Removed assert descriptions
* Used common/countdown instead of simple int
* Capitalized initial letter in example comment
@miles-po

This comment has been minimized.

Contributor

miles-po commented Feb 12, 2018

Addressed comments.

@BridgeAR

LGTM with a nit.

const test_res_body = 'other stuff!\n';
let processing_count = 0;
const countdown = new Countdown(3, common.mustCall(() => server.close()));

This comment has been minimized.

@BridgeAR

BridgeAR Feb 12, 2018

Member

The Countdown is already a mustCall the wrapping is therefore not necessary :-)

This comment has been minimized.

@miles-po

miles-po Feb 12, 2018

Contributor

Done

@BridgeAR

This comment has been minimized.

Member

BridgeAR commented Feb 12, 2018

@mcollina PTAL

@mcollina

LGTM

@miles-po

This comment has been minimized.

Contributor

miles-po commented Feb 14, 2018

Failing on the markdown linter for docs (which is not invoked by make -j4 test). Looks like it's only installed after you run make link-md-build. I don't think this is mentioned in the contrib docs. (Or at least I didn't see it.)

Sorry about the hiccup. Markdown warning fixed.

@mcollina

This comment has been minimized.

@miles-po

This comment has been minimized.

Contributor

miles-po commented Feb 14, 2018

This is in the failing test
java.lang.NoClassDefFoundError: Could not initialize class jenkins.model.Jenkins$MasterComputer
https://ci.nodejs.org/job/node-test-commit-linux-containered/nodes=ubuntu1604_sharedlibs_openssl102_x64/2233/console

@mcollina

This comment has been minimized.

Member

mcollina commented Feb 15, 2018

Landing.

@mcollina

This comment has been minimized.

Member

mcollina commented Feb 15, 2018

Landed as baf8495.

@mcollina mcollina closed this Feb 15, 2018

mcollina added a commit that referenced this pull request Feb 15, 2018

http: process 100, 102-199 according to specs.
Adding ServerResponse.writeProcessing to send 102 status codes.

Added an `'information'` event to ClientRequest to handle
1xx status codes except 101 Upgrade.
101 Upgrade is excluded due to its non-informational
processing according to RFC7231, Section 6.2.2.

This affects several modules downstream that use the http
module, e.g., node-fetch, all of whom violate HTTP RFCs
due to this module. As such, this could introduce a
breaking change for downstream if HTTP standards were
ignored in an ad-hoc fashion.

See also RFC2518 RFC8297.

PR-URL: #18033
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>

MayaLekova added a commit to MayaLekova/node that referenced this pull request May 8, 2018

http: process 100, 102-199 according to specs.
Adding ServerResponse.writeProcessing to send 102 status codes.

Added an `'information'` event to ClientRequest to handle
1xx status codes except 101 Upgrade.
101 Upgrade is excluded due to its non-informational
processing according to RFC7231, Section 6.2.2.

This affects several modules downstream that use the http
module, e.g., node-fetch, all of whom violate HTTP RFCs
due to this module. As such, this could introduce a
breaking change for downstream if HTTP standards were
ignored in an ad-hoc fashion.

See also RFC2518 RFC8297.

PR-URL: nodejs#18033
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment