fixing RealtimeSleepTimer to work in concurrent threads

[theBoatman]

The method RealtimeSleepTimer.sleep() does not work when called in multiple threads. This was not a problem when it was introduced, but since SealedSender there are two websocketConnetions, which both have a KeepAlive-Thread using RealtimeSleepTimer.sleep() concurrently, which leads to an Exception in many (perhaps all) gcm free devices every 55 seconds. This leads to several problems, including high energy usage or unreliable connection.

The exception is:
WebSocketConnection: java.lang.IllegalArgumentException: Receiver not registered: org.whispersystems.signalservice.api.util.RealtimeSleepTimer$AlarmReceiver@ab5dd6e

This exception is referenced in several issues of Signal-Android:
signalapp/Signal-Android#8658
signalapp/Signal-Android#8519
signalapp/Signal-Android#8402
signalapp/Signal-Android#8217

This patch resolves this exception.

There is already another pull request by @dpapavas (#62), which among other things tries to address this problem. I made a seperate request for two reasons:

1. I think @dpapavas tries to patch too many things at once, so the patch is difficult to apply because there are a lot of changes.
2. Although Detect and recycle broken connections on non-GCM devices #62 will solve the problem in the current situation, I am quite sure that it will not work as soon as there are more threads with different timings calling RealtimeSleepTimer.sleep(), so my solution should be more general and permanent.

If you plan to merge #62 in near future, you can close this request here as it is not needed. For gcm free users this is a very urgent problem, so it would be great if you could merge #62 or this patch here soon. I tried to stay as close as possible to the original code to make merging easy.

I have provided a compiled version of this patch for testing:
https://github.com/theBoatman/fixing-signal/blob/master/Signal-website-release-4.36.2-sleepfix.apk

There is also a forum thread discussing the problem:
https://community.signalusers.org/t/very-high-battery-drain-on-4-34-8-without-google-play-services/6536

[jmue]

Just two notes:

• Is there a necessity to have another try/catch around sleep? Did you encounter a exception there?
• Do you know of any downside to not just use Thread.currentThread().getId() instead of actionId as the additional alarm intent identifier?

[theBoatman]

@jmue Thanks for your review.

To the try/catch: Yes, the exception I fixed was thrown in that area of the code. I fixed the cause for it, but for the case another execption is thrown I added the try/catch to fix it permanently, as the calling code does not handle exceptions very well.

And to the thread id: I intentionally did not use the thread id as identifier because I wanted to use as less different identifier as possible and reuse them frequently. (The threads are recreated frequently and therefore the thread id changes often.) Alarms are monitored and logged by android and other apps and using a lot of different identifier would clutter the statistics.

[jmue]

The documentaion on ConcurrentLinkedQueue<> states:

public boolean add(E e)
Inserts the specified element at the tail of this queue. As the queue is unbounded, this method will never throw IllegalStateException or return false.

So actionId will always be 0, which matches my observations with additional logs enabled.

And to the thread id: I intentionally did not use the thread id as identifier because I wanted to use as less different identifier as possible and reuse them frequently. (The threads are recreated frequently and therefore the thread id changes often.) Alarms are monitored and logged by android and other apps and using a lot of different identifier would clutter the statistics.

OK, I get the point, which is valid. But introducing a addtional queue would clutter code.

[theBoatman]

Small update: I commited a fix for the problem @jmue had found. (My test builds contain this fix since over a month, so it is already well tested.)

There is a thread in the signal user forum with several debug logs proving that the requested change works and reduces problems.

I am using this since more than two months with no problems, and there are other users that use my test builds too. I also got a lot of positive feedback on this.

From my point of view this is perfectly ready for merging and would help a lot of people. Please accept his request or tell me what else is needed to get this solved.

[yeru-fixs]

@jmue any chance this issue will be fixed anytime soon. @theBoatman patched version works fine - no issues, the app from the master branch does not. please let me know if you need any debug logs. i understand your concern against another queue, but i find it hard to believe the creators of such an important application for the community would rather go with defects and suggest to "register with google" to fix it. thank you for your hard work and let me know if i may be of any help so the difference between running official and theboatman's version is not 3 days worth of battery on my phone with my regular use.

[getzze]

The patched version from @theBoatman has also been working perfectly on my device with LineageOS for some months now, it would be great if it is accepted upstream.

[jmue]

@yeru-signal I am not a signal developer, so I am unable to accept pull requests - although I would like to see this merged.

[five-c-d]

please let me know if you need any debug logs

@yeru-signal , yes those would be helpful. Here is the place to upload them == https://community.signalusers.org/t/call-for-testing-fixing-realtimesleeptimer-to-work-in-concurrent-threads/7189 ideally what will help is if you can run theBoatman's patched APK and upload a debuglog#1 using it, run stock signal4android on the same handset and make another debuglog#2 using THAT. This gives us a mostly-apples-to-apples comparison. @getzze same request please. If you only have time/patience for uploading one debuglog instead of two, that is fine, better some than none :-)

p.s. @yeru-signal traditionally only the people who are employed by Signal™ Foundation use the suffix in their github-usernames. If you are not... yet! :-) at least... on the payroll, would you might changing your github-username please? Similarly-confusing username, from late last year == https://community.signalusers.org/t/how-many-devs-are-working-on-signal/4102/33

[yeru-fixs]

@jmue Sorry, my bad. thought you are from signal
@five-c-d Name changed, my apologies. Have several accounts, had to differentiate by purpose. Let me see what I can do about logs.

[LoveIsGrief]

Thank you very much for the continued updates in https://github.com/theBoatman/fixing-signal . I hope this gets integrated someday

[krathalan]

@moxie-signal @alan-signal @greyson-signal can any developers comment on this? Many people use a Google Apps-free Android and this PR has been open for four months.

[alan-signal]

Signal service, being one level removed is harder for us to patch.

This class is only constructed inside the app, and injected into SignalServiceMessageReceiver, so there's no need for it to be in the service. If you can create a pr only within the main Android app, that would be the first step to getting this merged in.

To be clear, you can copy this whole class into the app project, and later we will deal with removing this old version from the library.

To help make it clear, rename the class from RealtimeSleepTimer to AlarmSleepTimer.

In the app, change the reference here:

  @Override
  public @NonNull SignalServiceMessageReceiver provideSignalServiceMessageReceiver() {
    SleepTimer sleepTimer = TextSecurePreferences.isFcmDisabled(context) ? new AlarmSleepTimer(context) //make sure this is the in-app one
                                                                         : new UptimeSleepTimer();
    return new SignalServiceMessageReceiver(networkAccess.getConfiguration(context),
                                            new DynamicCredentialsProvider(context),
                                            BuildConfig.USER_AGENT,
                                            new PipeConnectivityListener(),
                                            sleepTimer);
  }

[theBoatman]

Thanks @alan-signal for your feedback. I have made the pull request as you specified. I hope this helps. If there is anything more I can do, then please let me know.

[alan-signal]

Merged in app repo.