Add memory leak examples for Android documentation#920
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive set of code snippets demonstrating common memory leak patterns in Android and Compose, such as retained UI callbacks, improper lifecycle management for Flow collection, and uncleared view bindings. Feedback includes a recommendation to use repeatOnLifecycle for Flow collection in the Activity example to ensure efficiency and consistency with best practices. Additionally, it was suggested to re-categorize the fragment view binding example under Pattern 1 for improved documentation accuracy.
| lifecycleScope.launch { | ||
| UserRepositoryDelayedRecommended.getUserData().collect { data -> | ||
| title = data | ||
| } | ||
| } |
There was a problem hiding this comment.
For collecting flows in the UI, it is recommended to use repeatOnLifecycle to ensure that the collection only occurs when the Activity is in a visible state (e.g., STARTED). This is more efficient and consistent with the best practices shown in the Fragment example earlier in this file.
lifecycleScope.launch {
repeatOnLifecycle(Lifecycle.State.STARTED) {
UserRepositoryDelayedRecommended.getUserData().collect { data ->
title = data
}
}
}
There was a problem hiding this comment.
looks like gemini detected the code smell we're trying to show as an example
| } | ||
| // [END android_memory_leak_delayed_work_recommended] | ||
|
|
||
| // Pattern 3: Example 2 - Fragment view binding is not cleared |
There was a problem hiding this comment.
This example (Fragment view binding not cleared) belongs to Pattern 1: UI Objects Retained Beyond Lifecycle rather than Pattern 3 (External Registrations). Pattern 3 typically involves listeners or observers registered with system services or external components. Correcting this label will improve the documentation's accuracy.
There was a problem hiding this comment.
I think I agree with this assessment actually should we remove? It seems fairly similar as Pattern 1, option2.
I don't think a 2nd example is needed
* Add Pattern 1: UI Objects Retained Beyond Lifecycle examples * Add Pattern 2: Background Work Outlives the UI examples * Add Pattern 3: Unreleased External Registrations examples * Include Compose and Fragment-specific memory management best practices * Ensure snippets are idiomatic and comply with Spotless formatting These examples illustrate common architectural pitfalls and their corresponding lifecycle-aware resolutions using Coroutines, Flows, and DisposableEffect.
sudeshim3
force-pushed
the
feature/memory-leak-snippets
branch
from
671a754 to
8e3a6cc
Compare
| } | ||
| } | ||
|
|
||
| /* In the Fragment/UI layer: |
There was a problem hiding this comment.
Instead of showing commented out code you should show the actual compileable code, also I think you could show this occuring within a view model instead of calling from activity to avoid other code smells.
| binding.name.text = user.name | ||
| } | ||
| */ | ||
| // [END android_memory_leak_repository_callback_recommended] |
There was a problem hiding this comment.
Recommend creating 2 code snippets here if the intention is to show repository.fetchUser() getting called. What I mean is it could look something like:
// [START android_memory_leak_repository_callback_recommended_pt1]
class UserRepositoryRecommended {
suspend fun fetchUser(): User {
return api.getUser()
}
}
// [END android_memory_leak_repository_callback_recommended_pt1]
class UserViewModel(private val repository: UserRepositoryRecommended) : ViewModel() {
private val _userState = MutableStateFlow<User?>(null)
val userState: StateFlow<User?> = _userState.asStateFlow()
// [START android_memory_leak_repository_callback_recommended_pt2]
fun loadUser() {
// viewModelScope automatically cancels if the user leaves the screen
viewModelScope.launch {
val user = repository.fetchUser()
_userState.value = user
}
}
// [END android_memory_leak_repository_callback_recommended_pt2]
}
| // Accepts a callback that might capture a destroyed UI Context | ||
| fun fetchUserDataWithDelay(onComplete: (String) -> Unit) { | ||
| repositoryScope.launch { | ||
| delay(5_000) |
There was a problem hiding this comment.
suggest adding some explanation that the delay is to emulate network or processing.
| object UserRepositoryDelayedRecommended { | ||
| // A clean, stateless flow with no callback parameters | ||
| fun getUserData(): Flow<String> = flow { | ||
| delay(5_000) |
There was a problem hiding this comment.
suggest adding some explanation that the delay is to emulate network or processing.
| } | ||
| } | ||
|
|
||
| /* In the Fragment/UI layer: |
There was a problem hiding this comment.
same recommendation to actually create the Activity (or ViewModel) that calls the repository.
| lifecycleScope.launch { | ||
| UserRepositoryDelayedRecommended.getUserData().collect { data -> | ||
| title = data | ||
| } | ||
| } |
There was a problem hiding this comment.
looks like gemini detected the code smell we're trying to show as an example
| } | ||
| // [END android_memory_leak_delayed_work_recommended] | ||
|
|
||
| // Pattern 3: Example 2 - Fragment view binding is not cleared |
There was a problem hiding this comment.
I think I agree with this assessment actually should we remove? It seems fairly similar as Pattern 1, option2.
I don't think a 2nd example is needed
* Refactor Repository examples to use actual ViewModel implementations instead of commented-out pseudo-code. * Split the recommended Repository callback snippet into pt1 and pt2. * Add explanatory comments to delay() calls clarifying their purpose to emulate network/processing.
3 participants
These examples illustrate common architectural pitfalls and their corresponding lifecycle-aware resolutions using Coroutines, Flows, and DisposableEffect.