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

Handle client timeout #22

Open
georgefst opened this issue Feb 14, 2021 · 8 comments
Open

Handle client timeout #22

georgefst opened this issue Feb 14, 2021 · 8 comments
Labels
bug Something isn't working

Comments

@georgefst
Copy link
Owner

georgefst commented Feb 14, 2021

We want to be able to kill threads if they don't respond to pings, and run onDroppedConnection. Without this, the Linux backend can end up with an unbounded number of left over devices created in /dev/input.

websockets recently added serverRequirePong (jaspervdj/websockets#199), which handles some of this logic, though it doesn't allow us to specify an action to run on timeout (EDIT: that might be fine - assuming it just causes an exception to be thrown, we'll end up running onDroppedConnection anyway).

Another issue is that servant-websockets doesn't allow us to get at ServerOptions or ConnectionOptions (EDIT: not true, we actually now do in 2d3ea6b).`

We could manually implement a workaround like the one discussed here, though it's pretty ugly.

@georgefst
Copy link
Owner Author

the Linux backend can end up with an unbounded number of left over devices

Actually, we shouldn't need to be able to attach an action here to solve this. We just need to kill the thread. Since the devices get closed on garbage collection.

@georgefst georgefst added the bug Something isn't working label Feb 16, 2021
@georgefst
Copy link
Owner Author

georgefst commented Aug 13, 2021

Another issue is that servant-websockets doesn't allow us to get at ServerOptions.

This shouldn't be a surprise. With Servant, we're running a WAI server, rather than using the simple runServer provided by the websockets library.

@georgefst
Copy link
Owner Author

In fact, based on discussions in yesodweb/wai#310 and yesodweb/wai#232, it would appear that WAI already sets all this up.

It's possible I only assumed all this was an issue because of composewell/streamly#1203.

@georgefst
Copy link
Owner Author

Yep, I think this is closed by d9929bd. We actually get visible errors now when e.g. the browser tab is closed.

But I'll keep this open until I've actually confirmed that timeout errors occur.

@georgefst
Copy link
Owner Author

georgefst commented Sep 10, 2021

But I'll keep this open until I've actually confirmed that timeout errors occur.

Okay, from what I've seen so far, I'm pretty sure they don't.

@georgefst
Copy link
Owner Author

We just need to kill the thread. Since the devices get closed on garbage collection.

I see no sign of this. There are no guarantees of promptness, but maybe the thread isn't killed?

@georgefst
Copy link
Owner Author

We should think about how we want to handle reconnections (especially quick ones, e.g. in the case of WiFi dropping). I guess we don't want to immediately kill the thread anyway.

@domenkozar
Copy link

See also https://github.com/digitallyinduced/ihp/blob/dbb4ec64fe7b460fd80041e0b7a2867d90529d78/IHP/WebSocket.hs#L124

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants