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.

Sign up for GitHub

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

Jump to bottom

http: improving expectation handling #2403

Closed
jasnell opened this issue Aug 16, 2015 · 6 comments
Closed

http: improving expectation handling #2403

jasnell opened this issue Aug 16, 2015 · 6 comments
Labels
good first issue Issues that are suitable for first-time contributors. http Issues or PRs related to the http subsystem.

Comments

@jasnell
Copy link
Member

jasnell commented Aug 16, 2015

PR nodejs/node-v0.x-archive#7132 proposed a possible improvement to Expect
handling in http but it never landed. There was a little bit of review, but the conversation
stalled out and the submitted (@alFReD-NSH) moved on to other things. The PR is not
going to be able to land, and it's not a major priority, but I wanted to capture the issue here
before closing the PR. There does not seem to be anything inherently wrong with the change,
it just does not appear to be a significant priority. Additional review is helpful tho.

/cc @indutny
@mscdex mscdex added the http Issues or PRs related to the http subsystem. label Aug 16, 2015
@Fishrock123 Fishrock123 added the good first issue Issues that are suitable for first-time contributors. label Aug 25, 2015
@gyzerok
Copy link

gyzerok commented Sep 18, 2015

@jasnell Can I try to get this one?

@mnot
Copy link
Contributor

mnot commented Sep 19, 2015

Note that there's a spec change here, see:
http://httpwg.github.io/specs/rfc7231.html#header.expect

A server that receives an Expect field-value other than 100-continue MAY respond with a 417 (Expectation Failed) status code to indicate that the unexpected expectation cannot be met.

...

Note: The Expect header field was added after the original publication of HTTP/1.1 [RFC2068] as both the means to request an interim 100 (Continue) response and the general mechanism for indicating must-understand extensions. However, the extension mechanism has not been used by clients and the must-understand requirements have not been implemented by many servers, rendering the extension mechanism useless. This specification has removed the extension mechanism in order to simplify the definition and processing of 100-continue.

@designfrontier
Copy link
Contributor

If there is no one else tackling this I'd be happy to grab it and get a PR together. @gyzerok are you still working on this?

@guyellis
Copy link

@designfrontier let me know if you need a hand with this if/when you pick it up.

@designfrontier
Copy link
Contributor

Ok will do, hoping to pick in the next week right after Christmas.

-Daniel

On Dec 21, 2015, at 6:20 PM, Guy Ellis notifications@github.com wrote:

@designfrontier let me know if you need a hand with this if/when you pick it up.

—
Reply to this email directly or view it on GitHub.

@designfrontier designfrontier mentioned this issue Dec 31, 2015
Closed
designfrontier added a commit to designfrontier/node that referenced this issue Jan 8, 2016
@designfrontier
http: improves expect header handling
810158c 
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
jasnell pushed a commit that referenced this issue Jan 13, 2016
@designfrontier @jasnell
http: improves expect header handling
d755432 
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 pushed a commit that referenced this issue Jan 18, 2016
@designfrontier @evanlucas
http: improves expect header handling
c171294 
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
Copy link
Contributor

Closing as #4501 has landed in master. Feel free to reopen if necessary.

scovetta pushed a commit to scovetta/node that referenced this issue Apr 2, 2016
@designfrontier
http: improves expect header handling
88f013f 
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
good first issue Issues that are suitable for first-time contributors. http Issues or PRs related to the http subsystem.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
8 participants
@mscdex @mnot @designfrontier @jasnell @evanlucas @Fishrock123 @gyzerok @guyellis