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 broken aiohttp websocket in chat bot client #271

Merged
merged 3 commits into from
Oct 17, 2023

Conversation

Latent-Logic
Copy link
Contributor

@Latent-Logic Latent-Logic commented Oct 15, 2023

It turns out that aiohttp WebSocket code does not recognize if the underlying connection is broken:
aio-libs/aiohttp#2309
As a twitch client we know we'll at minimum receive a PING request every 5 minutes even if no other channel updates are happening:
https://dev.twitch.tv/docs/irc/#keepalive-messages (I tested this and saw every a ping request always happened between every 4min 1sec to 4min 51sec)

If we miss a PING and fail to respond twitch considers the connection closed. Thus we can add a timeout to our websocket receive() call and if we've heard nothing in over 5 minutes our connection is broken and we should perform a reconnect. This has been tested running the bot, and then trying 3 different types of connection breaking:

  1. Disabling the network interface on my computer for 5 minutes
  2. Suspending laptop and resuming later
  3. Dropping related active NAT state on router (similar to rebooting router)

It turns out that aiohttp WebSocket code does not recognize if the
underlying connection is broken:
aio-libs/aiohttp#2309
As a twitch client we know we'll at minimum receive a PING request every
5 minutes even if no other channel updates are happening:
https://dev.twitch.tv/docs/irc/#keepalive-messages
(I tested this and saw every a ping request always happened between
every 4min 1sec to 4min 51sec)

If we miss a PING and fail to respond twitch considers the connection
closed. Thus we can add a timeout to our websocket receive() call and if
we've heard nothing in over 5 minutes our connection is broken and we
should perform a reconnect. This has been tested running the bot, and
then trying 3 different types of connection breaking:
1) Disabling the network interface on my computer for 5 minutes
2) Suspending laptop and resuming later
3) Dropping related active NAT state on router (similar to rebooting router)
@Teekeks
Copy link
Owner

Teekeks commented Oct 17, 2023

I feel like going within seconds of a undocumented time period for a timeout seems unreasonable close.

Since its undocumented twitch could decide at any moment to change that value to e.g. 6 minutes or more resulting in frequent reconnects for some small bots.

Putting the default value to e.g. 10 minutes and making that value configurable should be the minimum.

@Latent-Logic
Copy link
Contributor Author

Makes sense, I'll have it be a class variable settable in the init method.

@Latent-Logic
Copy link
Contributor Author

If you want I could make it an optional variable that defaults to None and thus keep the current "hang forever" behavior, but I do think it is better to default to a state where it will reconnect if it hangs after too long.

@Teekeks
Copy link
Owner

Teekeks commented Oct 17, 2023

having the timeout is a good idea, however it would also be a good idea to have None as a valid value which it currently is not due to the *60

@Latent-Logic
Copy link
Contributor Author

None is now valid, and changed the type from int to float as an int is a valid value for a float, and a user might want to specify 5.5 minutes or something if they want to give a different timeout (and WebSocketResponse.receive timeout is timeout: Optional[float] = None)

https://peps.python.org/pep-0484/#the-numeric-tower

Rather than requiring that users write import numbers and then use numbers.Float etc., this PEP proposes a straightforward shortcut that is almost as effective: when an argument is annotated as having type float, an argument of type int is acceptable

@Teekeks Teekeks merged commit 28dd91a into Teekeks:master Oct 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants