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

fix(websocket): ignore resource close error #9755

Conversation

lucacasonato
Copy link
Member

@lucacasonato lucacasonato commented Mar 11, 2021

It is possible that the WebSocket is already closed when we try to
close it with WebSocket#close or in the error or close events.
Currently this leads to an uncatchable promise rejection. This changes
this so that closing an already closed WebSocket is a noop.

Closes #9879
Closes #9958

Copy link
Member

@ry ry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be useful to have a test. Will the WPT cover this?

@lucacasonato
Copy link
Member Author

Possibly - this is an internal Deno error we are exposing to the user though. No web API should ever through a Deno.errors error.

@bartlomieju
Copy link
Member

This PR should probably be landed onto 1.8.3 branch?

@lucacasonato
Copy link
Member Author

I'm going to pull the check into a helper function. Then yeah - let's cherry-pick it for 1.8.3

It is possible that the WebSocket is already closed when we try to
close it with `WebSocket#close` or in the `error` or `close` events.
Currently this leads to an uncatchable promise rejection. This changes
this so that closing an already closed WebSocket is a noop.
@lucacasonato lucacasonato force-pushed the ignore_already_closed_websocket_error branch from 211bb66 to 933e399 Compare April 1, 2021 20:45
@aricart
Copy link
Contributor

aricart commented Apr 1, 2021

It is possible that the WebSocket is already closed when we try to
close it with WebSocket#close or in the error or close events.
Currently this leads to an uncatchable promise rejection. This changes
this so that closing an already closed WebSocket is a noop.

Making it a noop is a good idea, the websocket event gets called after close(), which can easily yield double closes.

@lucacasonato lucacasonato merged commit 0e72129 into denoland:main Apr 1, 2021
@lucacasonato lucacasonato deleted the ignore_already_closed_websocket_error branch April 1, 2021 22:55
lucacasonato added a commit that referenced this pull request Apr 1, 2021
It is possible that the WebSocket is already closed when we try to
close it with `WebSocket#close` or in the `error` or `close` events.
Currently this leads to an uncatchable promise rejection. This changes
this so that closing an already closed WebSocket is a noop.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants