search

Gwindow / WhatAndroid

The What.CD Android App
http://gwindow.github.com/WhatAndroid/
BSD 2-Clause "Simplified" License
100 stars 20 forks source link

Convert AsyncTasks to IntentService #34

Open yulin2 opened 9 years ago

yulin2 commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

@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.