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

Convert AsyncTasks to IntentService #34

Open
wants to merge 9 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@yulin2

yulin2 commented May 5, 2015

Hi WhatAndroid developers, I'm doing research on Android async programming, particularly on
AsyncTask and IntentService. AsyncTask can lead to memory leak and losing task result when GUI is recreated during task running (such as orientation change). This article describes the problems very well.

We discussed with some Android experts and they agree with this issue, and claim that AsyncTask can be considered only for short tasks (less than 1 second). However, using IntentService (or AsyncTaskLoader) can avoid such problems since their lifecycles are independent from Activity/Fragment.

In WhatAndroid, most of the AsyncTasks are declared as non-static inner classes of Activity/Fragment. So the above issue can occur (especially for relatively long tasks).

I refactored 9 AsyncTasks in WhatAndroid to IntentService (in 9 commits), with the help of a refactoring tool we developed. Do you think using IntentService should be better in some of these 9 scenario? Do you want to merge some commits in this pr? Thanks.

@Twinklebear

This comment has been minimized.

Show comment
Hide comment
@Twinklebear

Twinklebear May 6, 2015

Collaborator

This is an interesting set of changes, AsyncTask can definitely be a pain to deal with and I think you're right that these tasks are a good fit for migration to IntentServices. There are a few things I would want to change though, I don't think the user feedback will be as good with this refactoring.

For example, since the receivers are unregistered when the fragment detaches those that show a toast will no longer show one, resulting in the user not getting feedback on the success/failure of the action if they navigate away from the fragment while the task is running. This wouldn't happen with the AsyncTask since it would outlive the fragment and run in the background, eventually showing the toast. Although the AsyncTasks also rely on getActivity which if we leave the activity may not be valid anymore, causing the same issue.

However an idea for moving off AsyncTask and all their other annoyances is pretty nice, I don't think I'll merge this pull request but I will migrate over to IntentServices when I have some time. I guess what I'd want for these is for them to outlive the fragment & activity that spawned them (eg. if we're on a slow mobile connection the task may take a while) and then show a notification when they finally finish and maybe unregister themselves.

Collaborator

Twinklebear commented May 6, 2015

This is an interesting set of changes, AsyncTask can definitely be a pain to deal with and I think you're right that these tasks are a good fit for migration to IntentServices. There are a few things I would want to change though, I don't think the user feedback will be as good with this refactoring.

For example, since the receivers are unregistered when the fragment detaches those that show a toast will no longer show one, resulting in the user not getting feedback on the success/failure of the action if they navigate away from the fragment while the task is running. This wouldn't happen with the AsyncTask since it would outlive the fragment and run in the background, eventually showing the toast. Although the AsyncTasks also rely on getActivity which if we leave the activity may not be valid anymore, causing the same issue.

However an idea for moving off AsyncTask and all their other annoyances is pretty nice, I don't think I'll merge this pull request but I will migrate over to IntentServices when I have some time. I guess what I'd want for these is for them to outlive the fragment & activity that spawned them (eg. if we're on a slow mobile connection the task may take a while) and then show a notification when they finally finish and maybe unregister themselves.

@tonyjhuang

This comment has been minimized.

Show comment
Hide comment
@tonyjhuang

tonyjhuang May 6, 2015

Collaborator

If showing toasts is your concern @Twinklebear, you're right in that IntentServices are not the right component to use in this case. However, it's just a hop and a skip away to change the IntentServices to Services and manage their lifecycles manually (rather than relying on the system to finish them when the intent is completed).

In using Services, their lifecycle is independent of the Activity lifecycle, and you have the added bonus of being able to rebind on orientation change :)

Collaborator

tonyjhuang commented May 6, 2015

If showing toasts is your concern @Twinklebear, you're right in that IntentServices are not the right component to use in this case. However, it's just a hop and a skip away to change the IntentServices to Services and manage their lifecycles manually (rather than relying on the system to finish them when the intent is completed).

In using Services, their lifecycle is independent of the Activity lifecycle, and you have the added bonus of being able to rebind on orientation change :)

@yulin2

This comment has been minimized.

Show comment
Hide comment
@yulin2

yulin2 May 6, 2015

But does the toast issue also occur for AsyncTask? Once Fragment is detached, getActivity will return null. So if the AsyncTask finishes after Fragment is detached, Toast.makeText in onPostExecute will throw NPE. That's why we have to unregister once a fragment is detached, right?

Also, if we register/unregister in onAttach/onDetach, we won't miss the Toast during orientation change, because onAttach is always called and the receiver will be registered. What do you think?

yulin2 commented May 6, 2015

But does the toast issue also occur for AsyncTask? Once Fragment is detached, getActivity will return null. So if the AsyncTask finishes after Fragment is detached, Toast.makeText in onPostExecute will throw NPE. That's why we have to unregister once a fragment is detached, right?

Also, if we register/unregister in onAttach/onDetach, we won't miss the Toast during orientation change, because onAttach is always called and the receiver will be registered. What do you think?

@Twinklebear

This comment has been minimized.

Show comment
Hide comment
@Twinklebear

Twinklebear May 6, 2015

Collaborator

@yulin2 right, if we leave the activity the AsyncTask's toast also won't show up. So maybe @tonyjhuang 's suggestion of using Services might be the best overall. I'll have to think more about what I really want, since we will lose toasts with AsyncTasks as well it could be that Services are the most reliable approach.

Collaborator

Twinklebear commented May 6, 2015

@yulin2 right, if we leave the activity the AsyncTask's toast also won't show up. So maybe @tonyjhuang 's suggestion of using Services might be the best overall. I'll have to think more about what I really want, since we will lose toasts with AsyncTasks as well it could be that Services are the most reliable approach.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment