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

Force inactive users to leave the room #935

Merged
merged 3 commits into from
Jul 10, 2018

Conversation

danxuliu
Copy link
Member

@danxuliu danxuliu commented Jun 4, 2018

Inactive guests (those that have not sent a ping in the last 30 seconds) are automatically cleaned from a room when the information for that room is got. In the same way, now inactive users are forced to leave the room too when the information for the room is got.

As leaving a room also resets its in call flag it is no longer needed to reset that flag using a background job.

How to test:

  • Open a conversation with another user
  • Join a call in that conversation
  • Kill the browser
  • Open the same conversation with the other user
  • Wait a while

Expected results:
The original user is no longer listed as in call (camera icon next to her name in the participant list) nor as in room (avatar not dimmed in the participant list).

Actual results:
If waited for less than five minutes, the original user is listed both as in call and as in room.

If waited for more than five minutes (and the background jobs run), the original user is not listed as in call, but she is still listed as in room.

});

foreach ($inactiveUsers as $inactiveUserId => $data) {
$room->leaveRoom($inactiveUserId);
Copy link
Member

Choose a reason for hiding this comment

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

Needs cast to (string) IIRC for users with only numbers as user id.

Copy link
Member

Choose a reason for hiding this comment

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

Also I think a "format" function is not the right place for such logic?

Copy link
Member Author

Choose a reason for hiding this comment

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

Needs cast to (string) IIRC for users with only numbers as user id.

I will add the cast.

Also I think a "format" function is not the right place for such logic?

I also think that it is not the right place, but I just put it where the kick out code for guest participants already was.

Copy link
Member

Choose a reason for hiding this comment

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

Ping, we want to do a release this week :)

@@ -219,6 +219,15 @@ protected function formatRoom(Room $room, Participant $participant = null) {
}
}

$inactiveUsers = array_filter($participants['users'], function($data) {
return $data['lastPing'] <= time() - 30 &&
Copy link
Member

Choose a reason for hiding this comment

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

This is a bit drastic? If one ping lags, you get killed out of the room.
The user tries 3 (or5) reconnects.

Copy link
Member Author

Choose a reason for hiding this comment

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

I just used the same timeout already being used for guests. Should I increase the timeout for both? Only for registered users? Or leave it like it is now?

Copy link
Member

Choose a reason for hiding this comment

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

Please use 100 seconds, so we are in 3 pings a 30 seconds + buffer

spreed/js/signaling.js

Lines 660 to 664 in db5861e

// If there is an error when pinging, retry for 3 times.
if (xhr.status !== 404 && this.pingFails < 3) {
this.pingFails++;
return;
}

danxuliu and others added 3 commits July 10, 2018 16:53
Inactive guests (those that have not sent a ping in the last 30 seconds)
are automatically cleaned from a room when the information for that room
is got. In the same way, now inactive users are forced to leave the room
too when the information for the room is got.

Although all inactive guests are cleaned all at once inactive users are
forced to leave the room one by one (each one with its own SQL query);
although it would be better to force all the inactive users to leave the
room at once this should not be a big problem, as users would usually
leave the room cleanly; only crashed clients or other exceptional
situations should cause a user to be inactive.

Signed-off-by: Daniel Calviño Sánchez <[email protected]>
When a user leaves a room the "in call" flag is also reset. As users are
now forced to leave the room when they are inactive it is no longer
needed to reset the flag using a background job.

Signed-off-by: Daniel Calviño Sánchez <[email protected]>
@nickvergessen nickvergessen force-pushed the force-inactive-users-to-leave-the-room branch from 1983bc5 to 7a50561 Compare July 10, 2018 14:53
@nickvergessen nickvergessen merged commit 9083102 into master Jul 10, 2018
@nickvergessen nickvergessen deleted the force-inactive-users-to-leave-the-room branch July 10, 2018 15:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants