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

Restructure connection checks to look for heartbeat timestamps #135

Merged
merged 1 commit into from
Oct 25, 2024

Conversation

third774
Copy link
Collaborator

This PR removes the websocket connection checks in the alarm, and instead looks for a heartbeat timestamp to evaluate whether or not to consider a user "timed out". This should hopefully resolve the issue we've seen for a while where sometimes random users are "timed out" incorrectly.

Copy link

@nils-ohlmeier nils-ohlmeier left a comment

Choose a reason for hiding this comment

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

No major concerns with the change.

One bigger question though is: do we want to continue to base the decision if a participant is still in a given meeting or not based on the state of the WS alone?
I think this gets tricky (or is impossible) to look at the state of the users PeerConnection as well and react differently if the PeerConnection appears to be connected and alive, while the WS is in trouble. Maybe more of an idea for another future improvement.

app/durableObjects/ChatRoom.server.ts Outdated Show resolved Hide resolved
app/durableObjects/ChatRoom.server.ts Show resolved Hide resolved
app/durableObjects/ChatRoom.server.ts Outdated Show resolved Hide resolved
app/durableObjects/ChatRoom.server.ts Outdated Show resolved Hide resolved
@third774
Copy link
Collaborator Author

do we want to continue to base the decision if a participant is still in a given meeting or not based on the state of the WS alone?

This change removes that almost completely. We will now only remove a user if:

  1. The user sends userLeft message
  2. The state broadcast to a user fails (we might even remove this one TBH?)
  3. The cleanup sees that the latest heartbeat is too old

What are your thoughts on this?

@nils-ohlmeier
Copy link

do we want to continue to base the decision if a participant is still in a given meeting or not based on the state of the WS alone?

What are your thoughts on this?

Let's give it a try. We can come back to this topic as needed. :-)

@third774 third774 force-pushed the restructure-connection-checks branch from e0a7f00 to 630ea66 Compare October 25, 2024 17:00
@third774 third774 merged commit 79fcf2f into main Oct 25, 2024
4 checks passed
@third774 third774 deleted the restructure-connection-checks branch October 25, 2024 17:47
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