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 race condition which often prevented sending group call started message #5001

Conversation

AsamK
Copy link
Contributor

@AsamK AsamK commented Feb 7, 2021

First time contributor checklist:

Contributor checklist:

Description

The group call update message after starting a group call is currently only
sent in the onLocalDeviceStateChanged callback. But often the peekInfo is
not available yet when the connection state changes to Joined, effectively
preventing the group call update message to be sent.

This commit also sends the message in the onPeekChanged callback. It is still
only sent at most once, which is ensured by the updateMessageState check.

I'm not sure if ringrtc is behaving correctly here or if the peek info should always be available if the connection state is Joined.
Then this PR would be just a workaround for an underlying problem in ringrtc ...

Fixes #4915

Manually tested with Signal-Desktop on Linux (x86_64) and the Android app.

…essage

The group call update message after starting a group call is currently only
sent in the onLocalDeviceStateChanged callback. But often the peekInfo is
not available yet when the connection state changes to Joined, effectively
preventing the group call update message to be sent.

This commit also sends the message in the onPeekChanged callback. It is still
only sent at most once, which is ensured by the updateMessageState check.
@EvanHahn-Signal
Copy link
Contributor

Thanks for doing this.

My understanding, which may be wrong, is that Desktop fails to receive notifications that group calls have started; it successfully sends them. Are you finding cases where you can join the call without an eraId and the notification fails to send to a mobile device? (If you are, I'll double-check that, but this code looks correct.)

@AsamK
Copy link
Contributor Author

AsamK commented Feb 8, 2021

Thanks for looking into it.

In my tests, Desktop receives notification messages that group calls have started just fine, e.g. when starting a group call from android. It just doesn't display a desktop notification but it shows the group call started message in the group chat.

But when starting a group call from Desktop, it doesn't send a group call started message in most cases. Sometimes it works, but in most cases it didn't.
I debugged it by putting breakpoints in the onLocalDeviceStateChanged and onPeekChanged methods. When Desktop failed to send the group call message, the connection state changed to Joined before the peekInfo was available on the group call object.

The code looks fine under the assumption that the peekInfo (with eraId) is always available before the connection state changes to Joined, but that's not the behavior I'm seeing.

(and just curious, what does era stand for)

@EvanHahn-Signal
Copy link
Contributor

"Era" is just the word; it's not an abbreviation. It's effectively the group call's "version". For example, if I start a group call, that's era 1. You and two other people join—still era 1. Then everyone leaves, and a few minutes later, you start a new call. That's era 2.

Sorry for taking a long time to respond here. I plan to take a deeper look soon.

Copy link
Contributor

@EvanHahn-Signal EvanHahn-Signal left a comment

Choose a reason for hiding this comment

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

Sorry to take so long to get to this! Looks good.

@EvanHahn-Signal EvanHahn-Signal merged commit ea7a544 into signalapp:development Apr 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

Signal-Desktop does not send group call started message to other users
2 participants