mscdex / socksv5

SOCKS protocol version 5 server and client implementations for node.js
MIT License
400 stars 121 forks source link

Agent: handle socket errors while connecting to the proxy #11

Open someone--else opened 8 years ago

someone--else commented 8 years ago

Passing client socket to the request on error allows it to be handled (vs aborting the process because of the unhandled exception)

alchen commented 8 years ago

I was just checking out socksv5 and met with an uncaught exception, and this has helped. Thank you.

It'd be great If this can be merged, or someone can provide a pointer to another solution.

wsz87 commented 8 years ago

I agree It should be merged. It works for me too.

mscdex commented 8 years ago

I'm not sure this is the right approach. This PR seems to be passing a socket as "valid" whenever an error occurs. Is the point to allow the Agent implementation to be able to clean up the socket immediately after the error occurs? It seems like there may still be the possibility of the Agent to pass the socket out as valid for a short period of time?

Also while on the topic, I'm thinking of ditching the copy of node's internal http.Agent code for the async agent.createConnection() functionality which will arrive in node v6.0.0 and will simplify this code greatly.

stroncium commented 8 years ago

seems to be related #19

Armalon commented 7 years ago

Not sure if it's the right code to be merged with master, but it worked for me just fine. I was glad to be able to catch errors instead of application being dead. Any ideas how to catch "Authentication failed" error using another method?

GabrielLomba commented 4 years ago

I also encountered this error and made a change in the library that reemitted the error in the request object:

client.on('error', err=>req.emit('error', err));

This way, it allows Authentication Failed and other types of errors to be handled outside of the library.