http: improves expect header handling by designfrontier · Pull Request #4501 · nodejs/node · GitHub

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

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

Thanks and done

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

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.

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)?

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

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

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

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

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

@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

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

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

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

LGTM @nodejs/http

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>

Landed in d755432

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.

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

http: improves expect header handling by designfrontier · Pull Request #4501 · nodejs/node · GitHub

Uh oh!

Uh oh!

http: improves expect header handling - #4501

http: improves expect header handling#4501designfrontier wants to merge 1 commit intonodejs:masternodejs/node:masterfrom designfrontier:feature-improve-expectation-handling-httpdesignfrontier/node:feature-improve-expectation-handling-httpCopy head branch name to clipboard

Conversation

designfrontier commented Dec 31, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasnell commented Jan 4, 2016

Uh oh!

jasnell commented Jan 4, 2016

Uh oh!

designfrontier commented Jan 4, 2016

Uh oh!

designfrontier commented Jan 7, 2016

Uh oh!

jasnell commented Jan 7, 2016

Uh oh!

mscdex commented Jan 7, 2016

Uh oh!

designfrontier commented Jan 8, 2016

Uh oh!

mscdex commented Jan 8, 2016

Uh oh!

designfrontier commented Jan 12, 2016

Uh oh!

jasnell commented Jan 12, 2016

Uh oh!

dougwilson commented Jan 12, 2016

Uh oh!

jasnell commented Jan 13, 2016

Uh oh!

rvagg commented Jan 18, 2016

Uh oh!

designfrontier commented Jan 18, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

http: improves expect header handling#4501
designfrontier wants to merge 1 commit into
nodejs:masterfrom
designfrontier:feature-improve-expectation-handling-http