Skip to content
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

http: improves expect header handling #4501

Conversation

designfrontier
Copy link
Contributor

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 mscdex added the http Issues or PRs related to the http subsystem. label Dec 31, 2015
res._expect_continue = true;
if (EventEmitter.listenerCount(self, 'checkContinue') > 0) {
self.emit('checkContinue', req, res);
if (!util.isUndefined(req.headers.expect) &&
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks and done

@designfrontier designfrontier force-pushed the feature-improve-expectation-handling-http branch from ce42fcd to 25f7e6a Compare December 31, 2015 23:16
res._expect_continue = true;
if (EventEmitter.listenerCount(self, 'checkContinue') > 0) {
self.emit('checkContinue', req, res);
if (typeof req.headers.expect !== 'undefined' &&
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@designfrontier designfrontier force-pushed the feature-improve-expectation-handling-http branch from 25f7e6a to 2343cb2 Compare January 1, 2016 06:17
if (continueExpression.test(req.headers.expect)) {
res._expect_continue = true;

if (EventEmitter.listenerCount(self, 'checkContinue') > 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, I think that should be ok.

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

http.request(options).on('response', (res) => {
process.exit();
Copy link
Contributor

Choose a reason for hiding this comment

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

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 jasnell added the semver-minor PRs that contain new features and should be released in the next minor version. label Jan 4, 2016
@jasnell
Copy link
Member

jasnell commented Jan 4, 2016

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

@jasnell
Copy link
Member

jasnell commented Jan 4, 2016

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

@designfrontier
Copy link
Contributor Author

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

@designfrontier 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
Copy link
Contributor Author

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

@jasnell
Copy link
Member

jasnell commented Jan 7, 2016

@mscdex
Copy link
Contributor

mscdex commented Jan 7, 2016

@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 designfrontier force-pushed the feature-improve-expectation-handling-http branch from e7d2033 to 810158c Compare January 8, 2016 05:53
@designfrontier
Copy link
Contributor Author

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

@mscdex
Copy link
Contributor

mscdex commented Jan 8, 2016

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

@designfrontier
Copy link
Contributor Author

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

@jasnell
Copy link
Member

jasnell commented Jan 12, 2016

LGTM @nodejs/http

@dougwilson
Copy link
Member

Seems OK

jasnell pushed a commit that referenced this pull request Jan 13, 2016
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
Copy link
Member

jasnell commented Jan 13, 2016

Landed in d755432

@jasnell jasnell closed this Jan 13, 2016
@rvagg
Copy link
Member

rvagg commented Jan 18, 2016

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
Copy link
Contributor Author

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

evanlucas pushed a commit that referenced this pull request Jan 18, 2016
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>
evanlucas added a commit that referenced this pull request Jan 20, 2016
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
evanlucas added a commit that referenced this pull request Jan 21, 2016
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
mbland pushed a commit to 18F/pages-server that referenced this pull request Feb 1, 2016
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
mbland pushed a commit to 18F/pages-server that referenced this pull request Feb 1, 2016
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
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
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>
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
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
trentm added a commit to TritonDataCenter/manta-muskie that referenced this pull request Apr 14, 2020
…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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
http Issues or PRs related to the http subsystem. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants