tls: handle ECONNRESET on the underlying socket #35829
Conversation
Add an error handler to the underlying TLSWrap socket mostly for the occasional ECONNRESET that a Linux kernel might throw under heavy load Send a tlsClientError event in this case instead of an uncatcheable socket error Fixes: nodejs#35824
| @@ -1084,6 +1088,8 @@ function tlsConnectionListener(rawSocket) { | |||
|
|
|||
| socket[kErrorEmitted] = false; | |||
| socket.on('close', onSocketClose); | |||
| // Handle ECONNRESET | |||
| socket.on('error', onSocketError); | |||
There was a problem hiding this comment.
Isn't this 'error' event already handled by
Line 510 in 0ca8617
There was a problem hiding this comment.
Yes, you are right.
Yet, when the ECONNRESET error is triggered, there is no error handler.
I see there are a couple of cases where the handler is removed just before closing, so I guess it must have been one of those cases. I will dig further.
There was a problem hiding this comment.
That listener is removed when the 'secureConnection' event is emitted but at that point it is user responsibility to handle the error.
There was a problem hiding this comment.
The user has no access to the underlying socket?
There was a problem hiding this comment.
Yes they do, it's the "socket" that is passed to the handlers of the 'secureConnection' event.
|
I confirm, that error can be handled by the user |
Add an error handler to the underlying TLSWrap socket
mostly for the occasional ECONNRESET that a Linux
kernel might throw under heavy load
Send a tlsClientError event in this case instead of an
uncatcheable socket error
Fixes: #35824
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesTriggering this bug requires continuous heavy networking load, I am not sure if I should include a unit test