http: improves expect header handling #4501

designfrontier · 2015-12-31T15:48:18Z

Now returns a 417 error status or allows for an event listener on
the checkExpectation event. Before we were ignoring requests that
had misspelled 100-continue values for expect headers.

This is a quick port of the work done here:
nodejs/node-v0.x-archive#7132 by alFReD-NSH
with surrounding discussion here:
nodejs/node-v0.x-archive#4651

Refs: #2403

mscdex · 2015-12-31T16:20:20Z

lib/_http_server.js

-      res._expect_continue = true;
-      if (EventEmitter.listenerCount(self, 'checkContinue') > 0) {
-        self.emit('checkContinue', req, res);
+    if (!util.isUndefined(req.headers.expect) &&


Avoid the type-checking util.* functions in node core code.

Thanks and done

mscdex · 2016-01-01T01:24:51Z

lib/_http_server.js

-      res._expect_continue = true;
-      if (EventEmitter.listenerCount(self, 'checkContinue') > 0) {
-        self.emit('checkContinue', req, res);
+    if (typeof req.headers.expect !== 'undefined' &&


Just a nit, but checking directly against undefined (req.headers.expect !== undefined) instead of using typeof is preferred, when compared to the rest of the code base.

I appreciate the nit picks :-) should be fixed now

mscdex · 2016-01-01T09:06:11Z

lib/_http_server.js

+      if (continueExpression.test(req.headers.expect)) {
+        res._expect_continue = true;
+
+        if (EventEmitter.listenerCount(self, 'checkContinue') > 0) {


Also while we're in here, EventEmitter.listenerCount() is deprecated, so we should just use self.listenerCount('checkContinue'); instead and similarly for the other event below.

@mscdex There is one other use of the EventEmitter.listenerCount in the file. Should I update it as well, or just change it in this new code?

Not sure how tightly scoped y'all like to keep the PRs.

Sure, I think that should be ok.

evanlucas · 2016-01-04T12:18:24Z

test/parallel/test-http-expect-handling.js

+    }));
+
+    http.request(options).on('response', (res) => {
+      process.exit();


we generally try to avoid explicitly calling process.exit() in tests. Could you instead just close the server (which should make the process exit on it's own)?

jasnell · 2016-01-04T17:20:23Z

Marking semver-minor due to the addition of the new event

jasnell · 2016-01-04T17:21:20Z

Generally LGTM but @evanlucas is correct about not calling process.exit() in the test. Once that's resolved this looks good.

designfrontier · 2016-01-04T17:42:38Z

I'll try and get that knocked out this evening or tomorrow morning. Thanks for the feedback

designfrontier · 2016-01-07T15:47:35Z

This should be ready to rock and roll now. Tests have been rewritten to use server.close() and are more complete now :-)

jasnell · 2016-01-07T23:44:37Z

CI: https://ci.nodejs.org/job/node-test-pull-request/1161/

mscdex · 2016-01-07T23:49:35Z

@designfrontier the linter is complaining.... looks like const EventEmitter = require('events'); needs to be removed from lib/_http_server.js now that it's no longer being used.

Now returns a 417 error status or allows for an event listener on the `checkExpectation` event. Before we were ignoring requests that had misspelled `100-continue` values for expect headers. This is a quick port of the work done here: nodejs/node-v0.x-archive#7132 by alFReD-NSH with surrounding discussion here: nodejs/node-v0.x-archive#4651 Also updates all the instances of the deprecated EventEmitter.listenerCount to the current self.listenerCount. Most of these were in the new code ported over but there was another legacy instance. Refs: nodejs#2403

designfrontier · 2016-01-08T05:54:35Z

whoops eslint was acting up for me in sublime apparently. Fixed.

mscdex · 2016-01-08T06:12:19Z

I think the other CI test failure is unrelated, but here goes again: https://ci.nodejs.org/job/node-test-pull-request/1166/

designfrontier · 2016-01-12T20:31:29Z

Looks like it passed jenkins... anything else I need to do on this?

jasnell · 2016-01-12T20:48:34Z

LGTM @nodejs/http

dougwilson · 2016-01-12T22:07:23Z

Seems OK

Now returns a 417 error status or allows for an event listener on the `checkExpectation` event. Before we were ignoring requests that had misspelled `100-continue` values for expect headers. This is a quick port of the work done here: nodejs/node-v0.x-archive#7132 by alFReD-NSH with surrounding discussion here: nodejs/node-v0.x-archive#4651 Also updates all the instances of the deprecated EventEmitter.listenerCount to the current self.listenerCount. Most of these were in the new code ported over but there was another legacy instance. Refs: #2403 PR-URL: #4501 Reviewed-By: James M Snell <jasnell@gmail.com>

jasnell · 2016-01-13T16:06:29Z

Landed in d755432

rvagg · 2016-01-18T02:42:00Z

Nice work @designfrontier! I believe this is your first commit in to core, if so, welcome on board! I hope you stick around and find other ways to contribute.

designfrontier · 2016-01-18T02:44:45Z

It is! I plan to stick around and keep grabbing ticket when I have spare
time :-)
On Sun, Jan 17, 2016 at 7:42 PM Rod Vagg notifications@github.com wrote:

Nice work @designfrontier https://github.com/designfrontier! I believe
this is your first commit in to core, if so, welcome on board! I hope you
stick around and find other ways to contribute.

—
Reply to this email directly or view it on GitHub
#4501 (comment).

Now returns a 417 error status or allows for an event listener on the `checkExpectation` event. Before we were ignoring requests that had misspelled `100-continue` values for expect headers. This is a quick port of the work done here: nodejs/node-v0.x-archive#7132 by alFReD-NSH with surrounding discussion here: nodejs/node-v0.x-archive#4651 Also updates all the instances of the deprecated EventEmitter.listenerCount to the current self.listenerCount. Most of these were in the new code ported over but there was another legacy instance. Refs: #2403 PR-URL: #4501 Reviewed-By: James M Snell <jasnell@gmail.com>

Notable changes: * events: make sure console functions exist (Dave) #4479 * fs: add autoClose option to fs.createWriteStream (Saquib) #3679 * http: improves expect header handling (Daniel Sellers) #4501 * node: allow preload modules with -i (Evan Lucas) #4696 * v8,src: expose statistics about heap spaces (`v8.getHeapSpaceStatistics()`) (Ben Ripkens) #4463 * Minor performance improvements: - lib: Use arrow functions instead of bind where possible (Minwoo Jung) #3622 - module: cache stat() results more aggressively (Ben Noordhuis) #4575 - querystring: improve parse() performance (Brian White) #4675 PR-URL: #4742

In Node v5.5.0, the HTTP server became more diligent about sending a HTTP 417 Expectation Failed response if the `Expect` header was not `100-continue`. Since `RequestHelper.httpOptions` was creating requests with an empty `Expect` header, and did not define a `checkExpectation` event handler, the server returned 417 errors for these requests. Also updated `RequestHelper.sendRequest` to use the status code text as the error message if the error response body is empty. https://nodejs.org/en/blog/release/v5.5.0/ nodejs/node#4501 https://nodejs.org/api/http.html#http_event_checkexpectation https://www.w3.org/Protocols/rfc2616/rfc2616-sec10.html#sec10.4.18 https://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.20 https://www.w3.org/Protocols/rfc2616/rfc2616-sec8.html#sec8.2.3

Now returns a 417 error status or allows for an event listener on the `checkExpectation` event. Before we were ignoring requests that had misspelled `100-continue` values for expect headers. This is a quick port of the work done here: nodejs/node-v0.x-archive#7132 by alFReD-NSH with surrounding discussion here: nodejs/node-v0.x-archive#4651 Also updates all the instances of the deprecated EventEmitter.listenerCount to the current self.listenerCount. Most of these were in the new code ported over but there was another legacy instance. Refs: nodejs#2403 PR-URL: nodejs#4501 Reviewed-By: James M Snell <jasnell@gmail.com>

Notable changes: * events: make sure console functions exist (Dave) nodejs#4479 * fs: add autoClose option to fs.createWriteStream (Saquib) nodejs#3679 * http: improves expect header handling (Daniel Sellers) nodejs#4501 * node: allow preload modules with -i (Evan Lucas) nodejs#4696 * v8,src: expose statistics about heap spaces (`v8.getHeapSpaceStatistics()`) (Ben Ripkens) nodejs#4463 * Minor performance improvements: - lib: Use arrow functions instead of bind where possible (Minwoo Jung) nodejs#3622 - module: cache stat() results more aggressively (Ben Noordhuis) nodejs#4575 - querystring: improve parse() performance (Brian White) nodejs#4675 PR-URL: nodejs#4742

…t "Expect" header usage Many MPU tests were setting "Expect: application/json", a presumed accidental misspelling of the "Accept" header. The only valid value for the "Expect" header is "100-continue". Old muskie was using node 0.10. Sometime after 0.10 nodejs/node#4501 changed node behaviour to respond with 417 Expectation Failed if given a bogus "Expect" header.

mscdex added the http Issues or PRs related to the http subsystem. label Dec 31, 2015

mscdex reviewed Dec 31, 2015
View reviewed changes

designfrontier force-pushed the feature-improve-expectation-handling-http branch from ce42fcd to 25f7e6a Compare December 31, 2015 23:16

mscdex reviewed Jan 1, 2016
View reviewed changes

designfrontier force-pushed the feature-improve-expectation-handling-http branch from 25f7e6a to 2343cb2 Compare January 1, 2016 06:17

mscdex reviewed Jan 1, 2016
View reviewed changes

designfrontier force-pushed the feature-improve-expectation-handling-http branch from 2343cb2 to 20db3fe Compare January 1, 2016 18:55

evanlucas reviewed Jan 4, 2016
View reviewed changes

jasnell added the semver-minor PRs that contain new features and should be released in the next minor version. label Jan 4, 2016

designfrontier force-pushed the feature-improve-expectation-handling-http branch 2 times, most recently from 99b615d to e7d2033 Compare January 6, 2016 14:53

designfrontier force-pushed the feature-improve-expectation-handling-http branch from e7d2033 to 810158c Compare January 8, 2016 05:53

jasnell closed this Jan 13, 2016

evanlucas mentioned this pull request Jan 18, 2016

v5.5.0 Release Proposal (Stable) #4742

Merged

evanlucas mentioned this pull request Jan 20, 2016

http: improving expectation handling #2403

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

http: improves expect header handling #4501

http: improves expect header handling #4501

designfrontier commented Dec 31, 2015

mscdex Dec 31, 2015

designfrontier Dec 31, 2015

mscdex Jan 1, 2016

designfrontier Jan 1, 2016

mscdex Jan 1, 2016

designfrontier Jan 1, 2016

mscdex Jan 1, 2016

evanlucas Jan 4, 2016

jasnell commented Jan 4, 2016

jasnell commented Jan 4, 2016

designfrontier commented Jan 4, 2016

designfrontier commented Jan 7, 2016

jasnell commented Jan 7, 2016

mscdex commented Jan 7, 2016

designfrontier commented Jan 8, 2016

mscdex commented Jan 8, 2016

designfrontier commented Jan 12, 2016

jasnell commented Jan 12, 2016

dougwilson commented Jan 12, 2016

jasnell commented Jan 13, 2016

rvagg commented Jan 18, 2016

designfrontier commented Jan 18, 2016

http: improves expect header handling #4501

http: improves expect header handling #4501

Conversation

designfrontier commented Dec 31, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasnell commented Jan 4, 2016

jasnell commented Jan 4, 2016

designfrontier commented Jan 4, 2016

designfrontier commented Jan 7, 2016

jasnell commented Jan 7, 2016

mscdex commented Jan 7, 2016

designfrontier commented Jan 8, 2016

mscdex commented Jan 8, 2016

designfrontier commented Jan 12, 2016

jasnell commented Jan 12, 2016

dougwilson commented Jan 12, 2016

jasnell commented Jan 13, 2016

rvagg commented Jan 18, 2016

designfrontier commented Jan 18, 2016