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

AbortController but for Websockets #57

Closed
amills-vibeirl opened this issue Feb 11, 2024 · 7 comments
Closed

AbortController but for Websockets #57

amills-vibeirl opened this issue Feb 11, 2024 · 7 comments

Comments

@amills-vibeirl
Copy link

What is the issue with the WebSockets Standard?

We can abort outgoing HTTP requests

We should be able to abort outgoing WS connection requests are pending

See:
https://stackoverflow.com/questions/77971834/how-to-cleanly-close-abort-a-webscoket-connection-before-it-connects

@ricea
Copy link
Collaborator

ricea commented Feb 14, 2024

The WebSocket close() method can already be used to do this. If you don't like Chrome's behaviour of printing a console message when this happens you should file an issue at https://crbug.com/.

The proposed WebSocketStream API uses AbortController for cancelling the handshake already.

We are unlikely to add this to the WebSocket API because it can be easily emulated with a function like

function AbortableWebSocket(url, signal) {
  signal.throwIfAborted();
  const ws = new WebSocket(url);
  const abortHandler = () => ws.close();
  signal.addEventListener('abort', abortHandler);
  ws.addEventListener('open', 
    () => signal.removeEventListener('abort', abortHandler));
  return ws;
}

@ricea ricea closed this as not planned Won't fix, can't repro, duplicate, stale Feb 14, 2024
@amills-vibeirl
Copy link
Author

amills-vibeirl commented Feb 14, 2024

ok that's nice @ricea , thank you

@bergus
Copy link

bergus commented Feb 14, 2024

You meant

  ws.addEventListener('close', () => signal.removeEventListener('abort', abortHandler));
  //                   ^^^^^

right?

@amills-vibeirl
Copy link
Author

.... waiting for Mr Rice

@ricea
Copy link
Collaborator

ricea commented Feb 18, 2024

You meant

  ws.addEventListener('close', () => signal.removeEventListener('abort', abortHandler));
  //                   ^^^^^

right?

No, my use of the open event was intentional, since I assume the aim is to abort the handshake only if it is still pending, and not close the WebSocket if the handshake has succeeded.

While it is tidy to remove the event listener when the WebSocket is closed, it's not strictly necessary, as calling close() on a WebSocket that is already closed does nothing.

@bergus
Copy link

bergus commented Feb 18, 2024

The proposed WebSocketStream API uses AbortController for cancelling the handshake already.

Oh I didn't read that closely enough, thanks for the re-emphasis:

I assume the aim is to abort the handshake only if it is still pending, and not close the WebSocket if the handshake has succeeded.

I don't think that's particularly useful. When I want to discard/cancel/abort a WebSocket connection, I don't care about its current status. I want it torn down, regardless whether the handshake has already been completed or not. (And what about race conditions where the handshake completion has already been received but not yet processed when the signal is aborted?)
I mean, it's great that with WebSocketStream now we also get the capability to abort a handshake and not just to .close() an already-established connection, but I think there should be a single method - preferably the CancelSignal option - to trigger both, depending on the underlying connection status. I don't want to have to write a conditional if (socket.readyState == 0) controller.abort() else if (socket.readyState == 1) socket.close() myself.

Sure it's also nice to have extra methods for more precise control, like .close() and .abortHandshake(), or closeSignal and abortHandshakeSignal, but the default should be the simpler and more general method.

While it is tidy to remove the event listener when the WebSocket is closed, it's not strictly necessary

It's necessary to avoid memory leaks, as the abort signal may be reused and around for much longer than the web socket.

@ricea
Copy link
Collaborator

ricea commented Feb 19, 2024

I don't think that's particularly useful. When I want to discard/cancel/abort a WebSocket connection, I don't care about its current status. I want it torn down, regardless whether the handshake has already been completed or not.

The close() method already does this. I don't think we need an additional mechanism.

(And what about race conditions where the handshake completion has already been received but not yet processed when the signal is aborted?)

Race conditions are an issue, yes, particularly if the main thread is busy.

It's necessary to avoid memory leaks, as the abort signal may be reused and around for much longer than the web socket.

True.

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

No branches or pull requests

3 participants