-
Notifications
You must be signed in to change notification settings - Fork 58
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
agentkeepalive detecting ECONNRESET #25
Comments
But because you are not listening on 'error' the process will crash before 'close' will get emitted. I added a listener locally to lib/_http_agent.js on line 233 on 'error' and have caught it once and not crashed. Since it only happens sporadicly, I don't know how to reproduce it. The stack trace isn't very helpful:
|
agentkeepalive don't handle socket error event. |
Hello, I have the same use case, using agentkeepalive together with aws/dynamodb. events.js:72 is what I got from logs. |
I was able to get around this by making the following changes: In _http_agent.js around line 260, add this listener to the socket: s.on('error', function(err) { Then in agent.js, I catch the error and log it (around line 70): self.on('error', function(err) { |
Workaround without change the source code: var createSocket = Agent.prototype.createSocket;
Agent.prototype.createSocket = function(req, options) {
var socket = createSocket.call(this, req, options);
socket.on('error', function(err) {
this.closeSocketCount++;
}.bind(this));
return socket;
}; ECONNRESET can easily reproduce: just shutdown a server after successful client request. |
I will add a default error listen to avoid |
Just to make it clear, you added a default error handler but errors will still bubble up at some point to the I am not sure to understand when does this unhandled error was triggered. In which cases? |
cc @fengmk2 |
the |
Ok sorry, so to be more precise, when was this unhandled error triggered? What were the possible unhandled errors, only ECONNRESET? Or was it possible to also get I am asking because we got a support request on one of our libs using agentkeepalive and wondered if this was linked. I talked about request |
The I think request object need to handle the |
I am trying to find a more detailed explanation of this issue, were you able to easily reproduce the error? Reading 24afa4a I cannot see a test case that is reproducing this behavior. To sump up: can you explain how from a user POV this error would happen. Using agentkeepalive for example and trying to make a request to an unknown hostname, I did get the error event (in 2.0.2). So I am not sure to understand the issue. |
@vvo https://github.com/nodejs/node/pull/4482/files ide sockets will fire error event and no body will handle it. |
Root cause is |
thanks for update |
from
Aryeh Schechter
email:Hello,
We recently started using agentkeepalive to pool our connections to aws and have been experiencing ECONNRESETs. We are using version 2.0.2 which has the fix for issue #23, but we are still getting these errors.
Why are you not listening for 'error' on the socket and handling it similar to 'close'? This would allow for the ability to catch the ECONNRESETs and retry the request without the hassle of the requiring library to handle 'uncaughtException' and trying to figure out where it came from.
Thanks,
Aryeh Schechter
www.glide.me
The text was updated successfully, but these errors were encountered: