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(web_socket): call `on_close` for all errors #8552

Merged
merged 2 commits into from Dec 16, 2019

Conversation

@stakach
Copy link
Contributor

stakach commented Dec 4, 2019

I occasionally see Errno on our webserver when calling run and all the clean-up code is in the on_close callback which is not called.

Consider the websocket handler in the stdlib, when run is called and an IO error such as Errno occurs on_close is never called and hence any references stored by the user defined code are leaked

Not something I'm able to replicate, hence the lack of tests, but I see it in the logs and our monitoring also confirms a leak.

I occasionally see `Errno` on our webserver when calling `run` and all the clean-up code is in the `on_close` callback which is not called.

Consider the [websocket handler](https://github.com/crystal-lang/crystal/blob/0e2e1d067af09e7b1e4573a7258c433e3cf8fa17/src/http/server/handlers/websocket_handler.cr#L43) in the stdlib, when run is called and an IO error such as `Errno` occurs `on_close` is never called and hence any references stored by the user defined code are leaked

Not something I'm able to replicate, hence the lack of tests, but I see it in the logs and our monitoring also confirms a leak.
@straight-shoota

This comment has been minimized.

Copy link
Member

straight-shoota commented Dec 4, 2019

Why not remove the exception's type restriction entirely and call on_close on every error? Technically, there might not be any other than IO::EOFError | Errno being raised, but it should still express the intention. We can assume that when ws.receive raises (for whatever reason) items the websocket is no longer in a working state.

@stakach

This comment has been minimized.

Copy link
Contributor Author

stakach commented Dec 4, 2019

Yeah good point @straight-shoota I'll update

@stakach stakach changed the title fix(web_socket): call `on_close` for all IO errors fix(web_socket): call `on_close` for all errors Dec 4, 2019
@stakach

This comment has been minimized.

Copy link
Contributor Author

stakach commented Dec 6, 2019

feel like reviewing @straight-shoota ?

@RX14
RX14 approved these changes Dec 7, 2019
@Sija

This comment has been minimized.

Copy link
Contributor

Sija commented Dec 16, 2019

IMO good candidate for 0.32.1.

@bcardiff bcardiff added this to the 0.32.1 milestone Dec 16, 2019
@bcardiff bcardiff merged commit 3938ef4 into crystal-lang:master Dec 16, 2019
6 checks passed
6 checks passed
ci/circleci: check_format Your tests passed on CircleCI!
Details
ci/circleci: test_darwin Your tests passed on CircleCI!
Details
ci/circleci: test_linux Your tests passed on CircleCI!
Details
ci/circleci: test_linux32_std Your tests passed on CircleCI!
Details
ci/circleci: test_preview_mt Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@bcardiff

This comment has been minimized.

Copy link
Member

bcardiff commented Dec 16, 2019

Sure, let's make it to 0.32.1.

bcardiff added a commit that referenced this pull request Dec 16, 2019
* fix(web_socket): call `on_close` for all IO errors

I occasionally see `Errno` on our webserver when calling `run` and all the clean-up code is in the `on_close` callback which is not called.

Consider the [websocket handler](https://github.com/crystal-lang/crystal/blob/0e2e1d067af09e7b1e4573a7258c433e3cf8fa17/src/http/server/handlers/websocket_handler.cr#L43) in the stdlib, when run is called and an IO error such as `Errno` occurs `on_close` is never called and hence any references stored by the user defined code are leaked

Not something I'm able to replicate, hence the lack of tests, but I see it in the logs and our monitoring also confirms a leak.

* fix(web_socket): call `on_close` for all errors
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.