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.

Sign up for GitHub

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

[Hack Week] A generic solution for resolving the ANRs caused by accessing local database #20937

Merged
merged 3 commits into from
Jun 5, 2024
Merged

[Hack Week] A generic solution for resolving the ANRs caused by accessing local database #20937

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
83c6a66
Create an alternative of AsyncTask for handling thread-switching req…
Jun 4, 2024
3eb24e4
Wrap the database-accessing code with AsyncTaskHandler
Jun 4, 2024
032d503
Add a comment
Jun 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 33 additions & 0 deletions WordPress/src/main/java/org/wordpress/android/datasets/AsyncTaskHandler.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
package org.wordpress.android.datasets

import kotlinx.coroutines.CoroutineScope
import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.launch
import kotlinx.coroutines.withContext

/**
* Helper class to handle async tasks by using coroutines
* @see <a href="https://github.com/wordpress-mobile/WordPress-Android/pull/20937">Introduction</a>
*/
object AsyncTaskHandler {
/**
* Load data in the background and handle the result on the main thread
*/
@JvmStatic
fun <T> load(backgroundTask: () -> T, callback: AsyncTaskCallback<T>) {
CoroutineScope(Dispatchers.IO).launch {
Copy link
Contributor

@thomashorta thomashorta Jun 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a few concerns about the usage of Coroutines here.

CoroutineScope

The CoroutineScope is being created ad hoc instead of using an existing Scope or Context/Job, which is usually attached to some lifecycle-aware component. This is essentially the same as using the GlobalScope, which should be avoided.

This means that the Job launched here is completely standalone, bypassing one of the biggest advantages introduced by coroutines: structured concurrency.

In practice, this means that even if the caller of the coroutine dies, the callback lambda will still be called (since the Scope is never canceled) which can cause issues if said callback references something that was destroyed.

TBH I'm not sure what would be the best solution here, but using an existing Scope that's close to the component that launches this Job would be preferred, all-in-all, thinking about this helper class, I'd say receiving the CoroutineScope via argument would make more sense so the caller can pass an appropriate Scope.

Hardcoded Dispatchers.IO

This is another thing that should be avoided, not only because it's hard to test, but also because, based on the name of this class (async TASK), it looks like this is supposed to be a generic "offload-to-background" helper.

This means that passing a backgroundTask that runs computational work could be expected, but that kind of work should be run in the Dispatchers.Default, while Dispatchers.IO should be used only for IO/waiting tasks. More info here.

The solution for this should probably be to receive the CoroutineDispatcher as an argument since the caller would be the one who knows what kind of task is being run.

Copy link
Contributor Author

@jarvislin jarvislin Jun 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@thomashorta Thank you for pointing out the potential issues in the current implementation.

I'd say receiving the CoroutineScope via argument would make more sense so the caller can pass an appropriate Scope.

Yeah this is also the way to handle the lifecycle stuff properly, will update it later.

Copy link
Contributor Author

@jarvislin jarvislin Jun 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the Hardcoded Dispatchers.IO, I'm not sure if I should receive the CoroutineDispatcher as an argument since the initial idea was to address issues related to accessing the database, opening it up to all dispatchers seems a bit over-engineered to me.

Therefore, I propose we keep this part as is and handle it through renaming for now. If a future need arises, we can then add the dispatcher argument. What do you think?

P.S. I think this helper is more related to the legacy code, especially in Java. Those codebase write in Kotlin should be able to handle the requirements without using this helper. So the code will use this helper might be less than we expected (and will be less after some refactors).

// handle the background task
val result = backgroundTask()

withContext(Dispatchers.Main) {
// handle the result on the main thread
callback.onTaskFinished(result)
}
}
}

interface AsyncTaskCallback<T> {
fun onTaskFinished(result: T)
}
}

76 changes: 40 additions & 36 deletions WordPress/src/main/java/org/wordpress/android/ui/reader/adapters/ReaderPostAdapter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import org.wordpress.android.R;
import org.wordpress.android.WordPress;
import org.wordpress.android.analytics.AnalyticsTracker;
import org.wordpress.android.datasets.AsyncTaskHandler;
import org.wordpress.android.datasets.ReaderPostTable;
import org.wordpress.android.datasets.ReaderTagTable;
import org.wordpress.android.fluxc.store.AccountStore;
Expand Down Expand Up @@ -371,44 +372,47 @@ private void toggleFollowButton(
return;
}

final boolean isAskingToFollow = !ReaderTagTable.isFollowedTagName(currentTag.getTagSlug());

final String slugForTracking = currentTag.getTagSlug();
AsyncTaskHandler.load(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand the usage here was more of an example of how to use this new AsyncTaskHandler but in this case (and I guess many other cases that are currently using DBs/network in the main thread) I feel the legacy code is in a bad state and the real fix should be a complete refactor.

For instance: this is a RecyclerView Adapter and it's dealing with data loading, which is clearly a bad practice, so a more coherent solution would be refactoring how the data is updated in the Adapter, probably moving the data loading to the appropriate ViewModel (which would have access to a proper CoroutineScope and ways of doing async work) and propagating the list state correctly to the Activity/Fragment containing the Adapter.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My approach is to first address the root cause with minimal changes and then look for opportunities to refactor. Refactoring involves more extensive changes, and dealing with legacy code introduces even more uncertainty. Therefore, I avoid making major changes right away unless I'm very confident or have plenty of time. It's a bit like a philosophical trade-off, but I agree that these related files need a complete refactor. In the long run, I believe the final outcome will be consistent.

() -> !ReaderTagTable.isFollowedTagName(currentTag.getTagSlug()),
isAskingToFollow -> {
final String slugForTracking = currentTag.getTagSlug();

ReaderActions.ActionListener listener = succeeded -> {
if (!succeeded) {
int errResId = isAskingToFollow ? R.string.reader_toast_err_adding_tag
: R.string.reader_toast_err_removing_tag;
ToastUtils.showToast(context, errResId);
} else {
if (isAskingToFollow) {
mReaderTracker.trackTag(
AnalyticsTracker.Stat.READER_TAG_FOLLOWED,
slugForTracking,
mSource
);
} else {
mReaderTracker.trackTag(
AnalyticsTracker.Stat.READER_TAG_UNFOLLOWED,
slugForTracking,
mSource
);
}
}
renderTagHeader(currentTag, tagHolder, true);
};

boolean success;
boolean isLoggedIn = mAccountStore.hasAccessToken();
if (isAskingToFollow) {
success = ReaderTagActions.addTag(mCurrentTag, listener, isLoggedIn);
} else {
success = ReaderTagActions.deleteTag(mCurrentTag, listener, isLoggedIn);
}

ReaderActions.ActionListener listener = succeeded -> {
if (!succeeded) {
int errResId = isAskingToFollow ? R.string.reader_toast_err_adding_tag
: R.string.reader_toast_err_removing_tag;
ToastUtils.showToast(context, errResId);
} else {
if (isAskingToFollow) {
mReaderTracker.trackTag(
AnalyticsTracker.Stat.READER_TAG_FOLLOWED,
slugForTracking,
mSource
);
} else {
mReaderTracker.trackTag(
AnalyticsTracker.Stat.READER_TAG_UNFOLLOWED,
slugForTracking,
mSource
);
if (isLoggedIn && success) {
renderTagHeader(currentTag, tagHolder, false);
}
}
}
renderTagHeader(currentTag, tagHolder, true);
};

boolean success;
boolean isLoggedIn = mAccountStore.hasAccessToken();
if (isAskingToFollow) {
success = ReaderTagActions.addTag(mCurrentTag, listener, isLoggedIn);
} else {
success = ReaderTagActions.deleteTag(mCurrentTag, listener, isLoggedIn);
}

if (isLoggedIn && success) {
renderTagHeader(currentTag, tagHolder, false);
}
);
}

private void renderXPost(int position, ReaderXPostViewHolder holder) {
Expand Down
Loading