Allow parser errors to bubble up to request

[mscdex]

Closes #292

[mikeal]

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]

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]

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

[mikeal]

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]

Done

[spollack]

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]

@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

[spollack]

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

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

[spollack]

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]

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

[spollack]

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

[mikeal]

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