Emit disconnected event instead of error when ECONNRESET #966
Conversation
ECONNRESET means the other side of the TCP conversation abruptly closed its end of the connection. If we get an ECONNRESET error from the proxy request and the socket is destroyed this means that the client has closed his connection, and emit this as an error can lead to confusion and hacks to filter that kind of message. I think that the best we can do is abort and emit another event, so if any caller wants to handle with that kind of error, he can by listen the disconnected event. http-party#813
|
@Deividy thanks for the contribution! I saw you did some investigation on removing the |
| @@ -132,7 +132,15 @@ web_o = Object.keys(web_o).map(function(pass) { | |||
| req.on('error', proxyError); | |||
|
|
|||
| // Error Handler | |||
| proxyReq.on('error', proxyError); | |||
| proxyReq.on('error', function (err) { | |||
| if (req.socket.destroyed && err.code === 'ECONNRESET') { | |||
There was a problem hiding this comment.
we should contain this logic in the proxyError function and change the error listener for req if we are going to keep it
There was a problem hiding this comment.
This should also be 2 spaces not 4 spaces for indentation.
|
Yep, the Thanks for the comments, I made the tweaks. |
|
Maybe missing a line of comment to explain why this function has an early return instead of calling callback or triggering error event. Otherwise looks very good, thank you for this PR! |
|
@jcrugzz good to go? |
|
@Deividy thank you for your work. Will merge this. id like to rebase the branch that removes teh |
Hello folks,
This is my first contribution, if there is anything wrong with my PR, please let me know.
We've been using node-http-proxy in our prod server since 3 years now and its working as a charm (thank you!), but everyday we got our log files full of
socket hang upmessages, so I come here dig code and found an issue for that: #813.To me, this looks like a bug also, we should not send as an error a disconnection between the client, so I made this PR using the info in the issue.
I'm not sure if emit a
disconnectedevent is the best approach, or if you guys want something more elaborate for that. Please, let me know your thoughts.