Fixed leak of CarAppLifecycleOwner #7669
Conversation
ChangelogFeatures
Bug fixes and improvements
Known issues
|
| @@ -172,6 +178,7 @@ internal class MapboxCopilotImpl( | |||
| fun stop() { | |||
| unregisterUserFeedbackCallback(userFeedbackCallback) | |||
| appLifecycleOwner.lifecycle.removeObserver(foregroundBackgroundLifecycleObserver) | |||
| appLifecycleOwnerCleanupAction() | |||
There was a problem hiding this comment.
appLifecycleOwnerCleanupAction is val and created once per MapboxCopilotInstance to cleanup resources acquired in constructor.
There was a problem hiding this comment.
Yes, but no one stops you from invoking stop twice. Now doing this action the second time is a no-op, but what if it become asymmetrical? I don't see any potential side-effects of nullifying it.
There was a problem hiding this comment.
@dzinad , do I understand your worries correctly: somebody can call internal method onStop a few times, which is not currently possible via public API, so that listener of activity lifecycle will be unregistered twice, where second unregistration does nothing?
I don't see any dangerous consequences with current approach. Can you please help me understand what exactly can go wrong there?
There was a problem hiding this comment.
I have a little bit another concern here. In case if MapboxNavigationApp.isSetup() is false, we call attachAllActivities only on MapboxCopilotImpl initialization, but call detachAllActivities when MapboxCopilotImpl.stop() is called. So there should be assumption that stop can be called only once. However, we're not protected from api misuse (even if it's internal class).
What if we will be assigning appLifecycleOwnerCleanupAction on start() call, and assign null on stop()?
There was a problem hiding this comment.
What if we will be assigning appLifecycleOwnerCleanupAction on start() call, and assign null on stop()?
In that case a different misuse is possible. If onStart is called twice, one of cleanup actions will be lost.
I don't think it's worth putting effort in protecting internal API from misuse if the price is code complexity.
There was a problem hiding this comment.
so that listener of activity lifecycle will be unregistered twice, where second unregistration does nothing?
It currently doesn't do nothing, it detaches all activities. It may result in being nothing, but I don't see any strong reason why we should rely on that. We can also add something else to appLifecycleOwnerCleanupAction, which won't be a no-op.
Ideally I'd move the setup to start and nullify it in stop (sth like what Dima suggested). And also add a check that start/stop is not invoked twice in a row (make the second invocation no-op).
But I don't find this critical. If we do that, we will reduce the risk of making an error in the future. But it's up to you whether you find this risk significant.
There was a problem hiding this comment.
Ideally I'd move the setup to start and nullify it in stop (sth like what Dima suggested). And also add a check that start/stop is not invoked twice in a row (make the second invocation no-op).
It's already implemented in MapboxCopilot. MapboxCopilot won't let MapboxCopilotImpl#onStart to be called 2 times. MapboxCopilotImpl could have been a private class if not unit tests which use it directly.
| val application = applicationContext as Application | ||
| attachAllActivities(application) | ||
| appLifecycleOwnerCleanupAction = { | ||
| detachAllActivities(application) |
There was a problem hiding this comment.
I think we can also add tests to make sure nothing is leaked anymore.
Codecov ReportAttention:
@@ Coverage Diff @@
## main #7669 +/- ##
============================================
- Coverage 74.12% 74.12% -0.01%
Complexity 6252 6252
============================================
Files 852 852
Lines 33696 33705 +9
Branches 4012 4012
============================================
+ Hits 24977 24983 +6
- Misses 7169 7172 +3
Partials 1550 1550
|
VysotskiVadim
added
the
needs backporting
VysotskiVadim
removed
the
needs backporting
VysotskiVadim
force-pushed
the
vv-cleanup-carapplifecycleowner
branch
from
c184c4d to
25c0597
Compare
Description
A customer complained that they see 14 log entries like
every time activity is paused. The same for resume.
During investigation I found out that every time
MapboxCopilotstarts, it creates a new instance ofCarAppLifecycleOwner, and subscribes for all activities changes without desubscribing from it. This PR added desubscription.