Ensure wakelocks are released on more exception paths. #1738
Conversation
There are a lot of places around xDrip+ where wakelocks are acquired but not released until the (generally inconsistently picked) timeout runs out. Many of those are deliberate (although probably not a good idea), but others occur by accident, on exception paths. This commit fixes a subset of the latter.
* There were a few cases where no try/finally block was used but there were multiple return statements that were each accompanied by a release of the wakelock. This is needlessly complex and error prone. I've replaced these with a try/finally block; note that even in case of a return statement being encountered in the try block, the finally block will still run.
* Many of these occur on background threads; when uncaught exceptions on background threads kill the app, this is okay - but these many threads should not hard code assumptions about the UncaughtExceptionHandler, which is a global setting. This commits introduces a helper class WakeLockThread that takes care of the acquisition/release of the wakelock, and ensures that the thread has a name (which can help during debugging); in some cases, this also allows dropping a try/finally block to avoid indentation in the source code. The presence of the new class should hopefully encourage more mindful handling of wakelocks in future code.
* There were also a handful of cases where there was a try/finally block, but the wakelock acquisition occurred a few statements before the try block; I've moved the wakelock acquisition to just before try block in order to make sure that it is always released.
Unrelatedly, I've also fixed a handful of needless constructions of Runnable objects for invocations of the form new Thread(new Runnable() { public void run() { ... } } which can instead just override Thread.run() instead (same behavior, but one less object).
I ran all Robolectric unit tests and confirmed that they still pass.
Potential work for follow-up commits:
- Potentially address the two TODOs and one FIXME that I've left in the code.
- Add convenience API to specify (TimeUnit timeUnit, long duration) instead
of (long millis) for more readable / less error-prone code.
- Add an AbstractWakeLockService class that takes care of acquiring/releasing
a wakelock during onStartCommand(), since xDrip+ has multiple independent
implementations of this behavior, some of which were buggy. That said,
the app will probably be killed during onStartCommand() only in extreme
cases, so acquiring the wakelock might be unnecessary in the first place.
|
@15characterlimi Thanks for catching this topic. It will take a wile to review it. What it makes harder is that there is just one commit with many changed files. But it looks like the same change is applied to several files. |
Would you prefer if I split this PR up into different "kinds" of changes, ie.
?
I only looked through usages of JoH.getWakeLock(). If that class/method is also duplicated for Wear, then that would explain it. Let me look into this some time outside of work time (I will have limited time to engage with this during the week, please bear with me). If this is all duplicated in Wear then the #1 goal should be to reduce/remove this duplication so that all future work doesn't need to be doubled-up. Would you agree? |
|
@15characterlimi No hurry. We all have limited time. BTW: Issue #1522 catches many of the code review aspects and the app/wear code duplication is also mentioned in #1733. The problem is that we cannot touch to many files at once. As more and more developers contribute to xDrip we have to get more organized. I'll propose structured collaboration, soon. Regarding to PRs affecting a larger number of files: I recommend creating commits as small as possible, atomic at best, and provide descriptive commit messages. That helps during review and allows cherry-picking. |
|
This PR isn't unreasonable in itself and looks like it has good practice behind it, but the main issues I see go like this:
|
The main goal of this commit is to set a precedent for good wakelock hygiene and to make it easy to ensure wakelocks are released in a timely and deterministic manner. Specifically:
In summary, the impact of each of the places touched by this commit is probably small, but (a) this commit fixes a lot of them, and (b) we don't know for sure that it isn't large because it's so difficult to track battery use back to a particular piece of sloppy coding. The idioms introduced by this commit aim to make the code more maintainable in the long run by setting precedent and by making it easier to write correct code. For example,
Not yet. I'd like to take a step back and ask you about the general engineering philosophy, because I think this will determine whether me trying to contribute to this project will deliver positive value vs. just be annoying for both of us. My personal impression is that xDrip+ has a lot of features, but the code supporting those features has become quite brittle and hard to maintain. Examples:
My personal perception is that xDrip+ is stuck in a local optimum that it can't get out of because the code is so brittle that no one dares to make any changes, beyond adding even more features. My personal perception is that xDrip+ has for years been stuck in a place where most effort was invested in more and more features, and very little effort was invested in keeping the existing code healthy (for example, by deleting dead code, refactoring sloppy or poorly abstracted code, or targeting recent versions of Android). My personal perception is that what xDrip+ needs most is not new features, but a higher code quality through static analysis (error-prone), more comprehensive unit tests, deduplication of code that was previously duplicated (Wear), ... Getting there will likely result in less stability in the short term as some things will invariably break as things get refactored, but it will result in much more stability and code quality in the long term. I personally value code quality and maintainability at least equally with feature set, and I think that this means that I could provide value. Does this match your philosophy of what you would like xDrip+s future to be? If not, then no problem, it's your project. But then I think it'd be good to know, because I think that my attempts to provide value would then just result in frustration for both of us. I used to maintain a codebase that had a lot more users than xDrip+ (actually, xDrip+ was one of those users...) and I'm definitely not a cowboy when it comes to making code changes. At the same time, I'm a strong believer in "No haunted graveyards" (example out of many excellent talks/articles). In my experience, subtle bugs that lurk below the surface can be worse over their lifetime than hard bugs because the hard bugs are found quickly and therefore short lived. From your and others' reactions to my bait in form of two pull requests, it seems to me that this approach to engineering might not be welcome here. If that's the case, then so be it. But it'd be good to know so we can each go our separate ways. WDYT? |
|
The main issue is integration testing. I don't believe there is really any automated way to unit test the interactions with the framework that occur over the (fragmented) Android ecosystem and xDrip commonly is interacting with remote devices which have their own quirks as well. This means that complete testing needs to be done with real handsets and real devices in my opinion. The existing code has been tested like this and so if its going to be changed then I have to weigh up the pros and cons of doing so and factor in the level of testing that has occurred. The changes proposed in this PR are relatively minor and I'm not suggesting that there are errors in it (although I haven't meticulously reviewed it either) but taking a strict engineering view when asking the question "does this break anything" the answer is that it hasn't been tested so we don't know for sure. This is why I asked that question. When someone produces changes for a particular part of the system which they are using every day, it is usually to address a specific need, like something is broken or a new feature is added and there is a greater level of confidence that it has been operationally tested (at least on the handsets they are using) - In my case I'm typically testing over a range of handsets with various chipsets and android versions and often live with multiple different users before merging the change. Last time I did an audit of wakelocks, what I found was that the only dangling one consuming anything significant was inside of google play services. This is why xDrip patches this at runtime to resolve that issue. The fact that this is necessary when it is google who are hounding developers to reduce background cpu usage tells you a lot about the chaotic nature of working with android and their engineering practices. One of the reasons for not targeting higher sdk versions is because of restrictions put in place by doing so that can break app behavior. In some situations this can be worked around with code changes and in others there is no replacement functionality (eg calling hidden api's to resolve framework bugs) Often google will introduce a replacement for something they are removing from a targetsdk level, provide documentation for it that is wrong (or manufacturers will integrate it incorrectly - hard to tell which) only to abandon it on the next major release. Such instability in the framework doesn't give so much incentive to adopt the new mechanisms while the old ones are still working. Eventually changes will be required as backward compatibility wanes in future android versions, a good example of this is package targeted broadcasts which are mandatory whichever android version is targeted. I do want to raise the minimum sdk level to android 4.4 (and in time 5.0) but last time I increased it there were complaints from users still using those handsets which is why that was reverted. I'm thinking enough time has passed now that such a change should be much safer to make. |
|
Thanks, that makes sense. There may be some common ground around better test coverage and/or refactoring code to be more testable that we can agree on. Let me think about this over the weekend and get back to you; perhaps I can think of some test coverage improvements that will make up for any error budget spent in the short term on refactorings/fixes such as in this PR here. Obviously a full integration test will not be possible without having all the other respective systems to integrate with (I don't have much other than my phone and a Dexcom G6, so an integration test in the general case is impossible for me), but it might possible in principle to extract an API boundary / abstraction in some places and then have unit tests that operate at that boundary. I haven't checked, but I wonder for example whether there are sufficient tests at the layer of the HTTP server; I've also been thinking of improving the UI of the appwidget (some extra lines could make it more readable IMO), and while I'm at it perhaps adding some screenshot-based integration tests. We'll see - I'll look into it. I'll also have to address @tolot27 's comments - I'll have to check for duplicate code that also needs to change, and might want to split this commit up into multiple that separately tackle different parts of the issue. Please hold on and I'll get back to you, hopefully by the end of the upcoming weekend (I also have a bunch of other things to deal with this weekend so can't 100% promise). I'll get back to you eventually. |
|
I'm not sure whether this PR would fix the NPEs which can be seen during unit tests, i. e. https://github.com/jamorham/xDrip-plus/pull/115/checks?check_run_id=2908921032#step:5:793: |
Probably not, the code still builds on top of JoH.getWakeLock() |
There are a lot of places around xDrip+ where wakelocks are acquired but not released until the (generally inconsistently picked) timeout runs out. Many of those are deliberate (although probably not a good idea), but others occur by accident, on exception paths. This commit fixes a subset of the latter.
There were a few cases where no try/finally block was used but there were multiple return statements that were each accompanied by a release of the wakelock. This is needlessly complex and error prone. I've replaced these with a try/finally block; note that even in case of a return statement being encountered in the try block, the finally block will still run.
Many of these occur on background threads; when uncaught exceptions on background threads kill the app, this is okay - but these many threads should not hard code assumptions about the UncaughtExceptionHandler, which is a global setting. This commits introduces a helper class WakeLockThread that takes care of the acquisition/release of the wakelock, and ensures that the thread has a name (which can help during debugging); in some cases, this also allows dropping a try/finally block to avoid indentation in the source code. The presence of the new class should hopefully encourage more mindful handling of wakelocks in future code.
There were also a handful of cases where there was a try/finally block, but the wakelock acquisition occurred a few statements before the try block; I've moved the wakelock acquisition to just before try block in order to make sure that it is always released.
Unrelatedly, I've also fixed a handful of needless constructions of Runnable objects for invocations of the form new Thread(new Runnable() { public void run() { ... } } which can instead just override Thread.run() instead (same behavior, but one less object).
I ran all Robolectric unit tests and confirmed that they still pass.
Potential work for follow-up commits:
of (long millis) for more readable / less error-prone code.
a wakelock during onStartCommand(), since xDrip+ has multiple independent
implementations of this behavior, some of which were buggy. That said,
the app will probably be killed during onStartCommand() only in extreme
cases, so acquiring the wakelock might be unnecessary in the first place.