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

Re-export HTTP parsers #766

Closed
wants to merge 1 commit into from
Closed

Re-export HTTP parsers #766

wants to merge 1 commit into from

Conversation

lllama
Copy link

@lllama lllama commented Feb 9, 2015

This change reverses 299cf84. Having HTTP parsers exported is useful when needing to debug HTTP connections (accessing raw buffers in particular). An example usage can be found here: http://miensol.pl/2014/03/23/nonintrusive-http-proxy-in-nodejs.html

NB: Commit message for 299cf84 mentions that a different name might be needed, as changes to the parsers' lifecycle may break existing usage.

This change reverses 299cf84. Having HTTP parsers exported is useful when needing to debug HTTP connections (accessing raw buffers in particular). An example usage can be found here: http://miensol.pl/2014/03/23/nonintrusive-http-proxy-in-nodejs.html

NB: Commit message for 299cf84 mentions that a different name might be needed, as changes to the parsers' lifecycle may break existing usage.
@vkurchatkin
Copy link
Contributor

-1. it should stay private

@ghost
Copy link

ghost commented Feb 9, 2015

I think it should be a configuration choice which defaults to not exporting.

@lllama
Copy link
Author

lllama commented Feb 9, 2015

The use case for this is to enable access to the raw HTTP messages, which are currently cleared when requests or responses have been parsed. Not being able to access the parsers seems to mean that accessing the messages will require passing connections through a raw socket (to buffer the messages ourselves) or reimplement our own parsers (which seems redundant given that there are robust parsers available).

Is my understanding correct? (quite possible that I'm completely wrong).

@bnoordhuis
Copy link
Member

@lllama I may be misunderstanding the use case but if you want the raw messages, can't you listen for 'data' events on the socket?

@lllama
Copy link
Author

lllama commented Feb 9, 2015

@bnoordhuis Listening on the "data" event will allow us to buffer the raw data but we will not be able to easily marry this up to a request object. e.g. if two requests are sent on the same socket then we will need to be able to reliably split them and associate them with the actual request object that we get from the "request" event.

@bnoordhuis
Copy link
Member

@lllama Okay, I think I get it now but after reading the article you linked to, I think I agree with @vkurchatkin that it's better if the parser list stays hidden.

The implementation details changed quite a bit since that article was written and will almost certainly change again. There is no promise of API stability; if you use parsers in your own code, you'll sooner or later end up shooting yourself in the foot.

If you really want to, you can use process.binding('http_parser').HTTPParser but you're probably better off using a module like https://github.com/creationix/http-parser-js.

@cjihrig
Copy link
Contributor

cjihrig commented Feb 9, 2015

I'm going to close this for now.

@cjihrig cjihrig closed this Feb 9, 2015
@lllama
Copy link
Author

lllama commented Feb 10, 2015

Thanks for the quick feedback on this PR. Very much appreciated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants