Allow parser errors to bubble up to request #293

Merged
merged 4 commits into from Aug 15, 2012

Projects

None yet

3 participants

@mscdex
Contributor
mscdex commented Jul 27, 2012

Closes #292

@mikeal
Member
mikeal commented Jul 27, 2012

in node 0.6.x+ these should always bubble up properly.

if they aren't this is a bad core problem.

we can't add indefinite error handlers on the socket object because it gets reused by other http requests in the agent pool.

@mscdex
Contributor
mscdex commented Jul 27, 2012

Unfortunately they do not seem to be bubbling up, at least on 0.8.x.

I've submitted a PR to fix this in node core, but I do not know if it'll make it into 0.8 or not.

@mscdex
Contributor
mscdex commented Jul 27, 2012

Ok, the leaking error handlers issue should be fixed now.

@mikeal
Member
mikeal commented Jul 27, 2012

this will still get called later and eventually leak if the socket is never closed by the agent.

it needs to be .once() and the listener needs to get removed when the request is done.

@mscdex mscdex Use .once() when listening for parser error
Also remove parser error listener when finished
205dfd2
@mscdex
Contributor
mscdex commented Jul 28, 2012

Done

@spollack
Contributor

Question on this. Using request 2.9.203 on node 0.6.20, I just hit a case where i get the following result:

events.js:48
throw arguments[1]; // Unhandled 'error' event
^
Error: Parse Error
at Socket.socketOnData (http.js:1288:20)
at TCP.onread (net.js:374:27)

So looks like an on('error', ...) handler is needed to catch this. The question is, would it make sense to have that handler live within the request lib, and map the emitted error event to a returned error passed to the user's callback function, as opposed to making the user also register an error handler and bubbling up to it? I see the thread above it following the second path, just curious about the rationale and looking to understand/learn. thanks!

@mscdex
Contributor
mscdex commented Aug 10, 2012

@spollack I've pretty much given up on this particular PR since I've submitted a PR to fix this in node core (it may not make it into node 0.8): nodejs/node-v0.x-archive#3777

@mscdex mscdex closed this Aug 10, 2012
@spollack
Contributor

@mscdex thanks.

So for now, until the node core PR gets picked up, what is the best practice for catching these types of errors when using request?

@mscdex
Contributor
mscdex commented Aug 11, 2012

@spollack you'll probably have to patch it yourself using a method similar to the one I presented in this PR

@spollack
Contributor

Thanks.

I think it would be good for request to pick up this change in the meantime, given that is is broken on at least node 0.6.20 (from my testing), and it sounds like from your testing @mscdex also in node 0.8.x. @mikeal what do you think? thanks

@mikeal
Member
mikeal commented Aug 15, 2012

if it's broken in 0.6 we need a fix in.

@spollack
Contributor

I forked and applied the PR above from @mscdex and it does solve the problem for me.

@mikeal mikeal reopened this Aug 15, 2012
@mikeal mikeal merged commit 07135d5 into request:master Aug 15, 2012
@mikeal
Member
mikeal commented Aug 15, 2012

let this marinate on master for a little while before a new release.

@zarenner zarenner referenced this pull request Jun 10, 2016
Closed

SSL error #1903

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment