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

fixing RealtimeSleepTimer to work in concurrent threads #70

Open
wants to merge 2 commits into
base: master
from

Conversation

@theBoatman
Copy link

commented Apr 3, 2019

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 #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

This comment has been minimized.

Copy link

commented Apr 9, 2019

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

This comment has been minimized.

Copy link
Author

commented Apr 10, 2019

@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

This comment has been minimized.

Copy link

commented Apr 10, 2019

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

This comment has been minimized.

Copy link
Author

commented May 22, 2019

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

This comment has been minimized.

Copy link

commented Jun 19, 2019

@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

This comment has been minimized.

Copy link

commented Jun 20, 2019

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

This comment has been minimized.

Copy link

commented Jun 20, 2019

@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 five-c-d referenced this pull request Jun 23, 2019
five-c-d added a commit to five-c-d/libsigsvc4j that referenced this pull request Jun 23, 2019
@five-c-d five-c-d referenced this pull request Jun 24, 2019
@five-c-d

This comment has been minimized.

Copy link

commented Jun 24, 2019

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

This comment has been minimized.

Copy link

commented Jun 25, 2019

@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

This comment has been minimized.

Copy link

commented Aug 9, 2019

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

@krathalan

This comment has been minimized.

Copy link

commented Aug 11, 2019

@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.

@Eskuero Eskuero referenced this pull request Aug 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.