Skip to content
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

Rework connect timeout handling #1100

Open
arthurschreiber opened this issue May 26, 2020 · 0 comments
Open

Rework connect timeout handling #1100

arthurschreiber opened this issue May 26, 2020 · 0 comments

Comments

@arthurschreiber
Copy link
Collaborator

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
Projects
None yet
Development

No branches or pull requests

3 participants