Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

http: Improve HTTP Expect header handling #7132

Closed
wants to merge 1 commit into from

Conversation

faridnsh
Copy link

Just following the spec:
http://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.20

Fixes #4651

Note that, Now we have the following pattern 3 times in the _http_server.js:

if (EventEmitter.listenerCount(self, 'someEventName') > 0) {
    self.emit('someEventName');
} else {
     // Do something as default
}

I think maybe we should move it to a function, though didn't have any good idea for the name of the function. Something like this. If you guys give me a good name, I'll refactor it.

I also see this pattern in userland too. Maybe it should be a event emitter prototype method name setDefaultListener that will set the default action when there's no listeners but won't do anything when there are listeners to a particular event.

@Nodejs-Jenkins
Copy link

Thank you for contributing this pull request! Here are a few pointers to make sure your submission will be considered for inclusion.

The following commiters were not found in the CLA:

  • Farid Neshat

You can fix all these things without opening another issue.

Please see CONTRIBUTING.md for more information

@tjfontaine
Copy link

I'm generally ok with this, but I want to solicit some feedback from others using this feature to know if this would be too much of a surprise.

On the pattern, you could describe it this way:

if (!self.emit('checkExpectation'...)) {
  res.writeHead(417);
  res.end();
}

I'm not really thrilled with the idea of the prototype method.

@jasnell
Copy link
Member

jasnell commented Aug 3, 2015

@alFReD-NSH ... still want this?

@faridnsh
Copy link
Author

faridnsh commented Aug 4, 2015

I actually don't need this for myself anymore. But should check with #4651

@jasnell
Copy link
Member

jasnell commented Aug 13, 2015

Ok, given the change I'm not sure if this would land in here or if it should be retargeted to nodejs/node. @alFReD-NSH , are you willing to take another look at this?

@faridnsh
Copy link
Author

I don't need this anymore. If you think this should be merged, then merge it if not, if it needs any changes, then close this, because at this moment I don't have time for this.

@jasnell
Copy link
Member

jasnell commented Aug 13, 2015

For now I'll mark it as a low priority and assign myself to it. If I'm able to get back to it I will, otherwise it'll just have to sit a while longer. Thanks @alFReD-NSH !

@jasnell
Copy link
Member

jasnell commented Aug 16, 2015

Opened a new issue in nodejs/node to track this. Will close this PR as it's not going to be able to land.

@jasnell jasnell closed this Aug 16, 2015
designfrontier added a commit to designfrontier/node that referenced this pull request Jan 8, 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
jasnell pushed a commit to nodejs/node 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 <[email protected]>
evanlucas pushed a commit to nodejs/node 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 <[email protected]>
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 <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants