You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
The way we perform timeouts inside tedious is fragile and often causes weird issues.
One good example is an interaction between connect timeouts and socket connect errors, as described in #782. The gist of that issue is that when the connect timeout happens, socket operations can still be in-flight, and by the time the ETIMEDOUT actually happens on the socket, the error can't be handled correctly anymore and we emit an error event on the Connection (which can cause the node process to crash unless handled).
This is happening because we're not correctly propagating the connect cancellation information from Connection to Connector and further down the actual connection strategy that is being used, and the underlying sockets.
One way this could be fixed is by adding a .cancel() method on Connector that would perform all the necessary cleanup. Implementing this seems to be complicated, because of all the async logic involved in handling the socket establishment. I tried implementing this, but really disliked the resulting code.
Since a few days ago, there's quite a bit of discussion about making the AbortController and AbortSignal pattern available. This pattern is available in browser to abort fetch requests, and the plan is to make the same pattern the default and preferred way to handle cancellation on async operations. Discussions can be found at nodejs/node#33527 and openjs-foundation/summit#273.
Even though AbortController is not part of NodeJS core yet, there are userland implementations (like abort-controller) that we could use in the meantime for improving our internal cancellation handling.
For connect timeout handling, we could make use of the AbortController and AbortSignal pattern and clean up rough edges.
We would have to extend the Connector class to take an AbortSignal argument on construction, which we could then pass down to the connection strategy instances. Inside the Connector class (and SequentialConnectStrategy and ParallelConnectStrategy), we could then either check AbortSignal.aborted and/or listen to the aborted event to correctly clean up sockets and other in-flight operations correctly.
Inside the Connection class, we'd create an AbortController before we create the Connector class and schedule the connect timer. The controller's signal can then be passed to the Connector, and when the connect timer fires, we can signal abortion by calling abort on the controller.
Handling cancellation this way should be much easier to understand and flexible than exposing .cancel methods, and should neatly fix issues like #782
The text was updated successfully, but these errors were encountered:
The way we perform timeouts inside tedious is fragile and often causes weird issues.
One good example is an interaction between connect timeouts and socket connect errors, as described in #782. The gist of that issue is that when the connect timeout happens, socket operations can still be in-flight, and by the time the
ETIMEDOUT
actually happens on the socket, the error can't be handled correctly anymore and we emit anerror
event on theConnection
(which can cause the node process to crash unless handled).This is happening because we're not correctly propagating the connect cancellation information from
Connection
toConnector
and further down the actual connection strategy that is being used, and the underlying sockets.One way this could be fixed is by adding a
.cancel()
method onConnector
that would perform all the necessary cleanup. Implementing this seems to be complicated, because of all the async logic involved in handling the socket establishment. I tried implementing this, but really disliked the resulting code.Since a few days ago, there's quite a bit of discussion about making the
AbortController
andAbortSignal
pattern available. This pattern is available in browser to abortfetch
requests, and the plan is to make the same pattern the default and preferred way to handle cancellation on async operations. Discussions can be found at nodejs/node#33527 and openjs-foundation/summit#273.Even though
AbortController
is not part of NodeJS core yet, there are userland implementations (likeabort-controller
) that we could use in the meantime for improving our internal cancellation handling.For connect timeout handling, we could make use of the
AbortController
andAbortSignal
pattern and clean up rough edges.We would have to extend the
Connector
class to take anAbortSignal
argument on construction, which we could then pass down to the connection strategy instances. Inside theConnector
class (andSequentialConnectStrategy
andParallelConnectStrategy
), we could then either checkAbortSignal.aborted
and/or listen to theaborted
event to correctly clean up sockets and other in-flight operations correctly.Inside the
Connection
class, we'd create anAbortController
before we create theConnector
class and schedule the connect timer. The controller'ssignal
can then be passed to theConnector
, and when the connect timer fires, we can signal abortion by callingabort
on the controller.Handling cancellation this way should be much easier to understand and flexible than exposing
.cancel
methods, and should neatly fix issues like #782The text was updated successfully, but these errors were encountered: