Skip to content

Rework connect timeout handling #1100

Open

Description

@arthurschreiber

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions