http: improves expect header handling

[designfrontier]

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]

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

[designfrontier]

Thanks and done

[mscdex]

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.

[designfrontier]

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

[mscdex]

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.

[designfrontier]

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

[mscdex]

Sure, I think that should be ok.

[evanlucas]

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.

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

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