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 #793

Closed
wants to merge 10 commits into
base: develop
from

Conversation

Projects
None yet
3 participants
@yulin2

yulin2 commented May 6, 2015

Hi AntennaPod developers, I'm a Ph.D. student and 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 AntennaPod, many 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 8 AsyncTasks in AntennaPod to IntentService (in separate commits), with the help of a refactoring tool we developed. I also mark some types as Serializable in order to put them into Intent. Do you think using IntentService should be better in some of these 8 scenario? Do you want to merge (some) commits in this pr? Thanks.

@mfietz

This comment has been minimized.

Show comment
Hide comment
@mfietz

mfietz May 6, 2015

Contributor

Wouldn't it be a lot easier to just use AsyncTaskLoaders?
Sending and receiving data that never actually leaves the fragment/activity through BroadcastReceivers seem like a code smell to me.

To be honest, I haven't really dug deep into your proposal, but what I examined so far appears to be a lot more complicated that it needs to be.
I may be completely wrong on this - I'm far from an expert on android's background processing.

By the way, most of our tasks read or write data from the database and should finish well under 100ms.
As soon as the AsyncTask finishes, the GC should be able to reclaim the destroyed activities memory, shouldn't it?

Contributor

mfietz commented May 6, 2015

Wouldn't it be a lot easier to just use AsyncTaskLoaders?
Sending and receiving data that never actually leaves the fragment/activity through BroadcastReceivers seem like a code smell to me.

To be honest, I haven't really dug deep into your proposal, but what I examined so far appears to be a lot more complicated that it needs to be.
I may be completely wrong on this - I'm far from an expert on android's background processing.

By the way, most of our tasks read or write data from the database and should finish well under 100ms.
As soon as the AsyncTask finishes, the GC should be able to reclaim the destroyed activities memory, shouldn't it?

@yulin2

This comment has been minimized.

Show comment
Hide comment
@yulin2

yulin2 May 6, 2015

Thanks!

AsyncTaskLoader can also solve the problem, though it's introduced after Android 3.0 (or you can use Android support v4), and it only supports activity and fragment. Also, actually I don't think AsyncTaskLoader is necessarily easier to use in all cases. AsyncTaskLoader will reuse the cached result (that comes from its first execution) even if you trigger it multiple times, unless you implement that deliverResult method properly based on your needs.

I agree either IntentService or AsyncTaskLoader makes the code more complex. So I didn't say
we should never use AsyncTask. For short tasks, it should be fine.

I sent this pr because I'm wondering in AntennaPod, are there any AsyncTasks need to be converted to IntentService (or AsyncTaskLoader). In other words, whether some of such changes can be useful for AntennaPod. For example, maybe some tasks are really long (e.g., more than one or two seconds)?

yulin2 commented May 6, 2015

Thanks!

AsyncTaskLoader can also solve the problem, though it's introduced after Android 3.0 (or you can use Android support v4), and it only supports activity and fragment. Also, actually I don't think AsyncTaskLoader is necessarily easier to use in all cases. AsyncTaskLoader will reuse the cached result (that comes from its first execution) even if you trigger it multiple times, unless you implement that deliverResult method properly based on your needs.

I agree either IntentService or AsyncTaskLoader makes the code more complex. So I didn't say
we should never use AsyncTask. For short tasks, it should be fine.

I sent this pr because I'm wondering in AntennaPod, are there any AsyncTasks need to be converted to IntentService (or AsyncTaskLoader). In other words, whether some of such changes can be useful for AntennaPod. For example, maybe some tasks are really long (e.g., more than one or two seconds)?

@mfietz

This comment has been minimized.

Show comment
Hide comment
@mfietz

mfietz May 8, 2015

Contributor

@yulin2 Just found this: https://github.com/square/leakcanary Might also be interesting for your research
I think we will integrate this soon (or at least play around with it). At least we will know where actual memory leaks are, no more guessing.

Contributor

mfietz commented May 8, 2015

@yulin2 Just found this: https://github.com/square/leakcanary Might also be interesting for your research
I think we will integrate this soon (or at least play around with it). At least we will know where actual memory leaks are, no more guessing.

@yulin2

This comment has been minimized.

Show comment
Hide comment
@yulin2

yulin2 May 9, 2015

Interesting stuff. Their example in README also shows a leak in AsyncTask:)

For short tasks, AsyncTask should be fine since the leak (if any) does not last very long.
The problem is not only leaks: if in AsyncTask you update a GUI that has already been destroyed, your update cannot be shown on screen (sometimes also result in NPE).

yulin2 commented May 9, 2015

Interesting stuff. Their example in README also shows a leak in AsyncTask:)

For short tasks, AsyncTask should be fine since the leak (if any) does not last very long.
The problem is not only leaks: if in AsyncTask you update a GUI that has already been destroyed, your update cannot be shown on screen (sometimes also result in NPE).

@TomHennen

This comment has been minimized.

Show comment
Hide comment
@TomHennen

TomHennen Aug 1, 2015

Contributor

We're going to look at other methods of solving this problem.

Contributor

TomHennen commented Aug 1, 2015

We're going to look at other methods of solving this problem.

@TomHennen TomHennen closed this Aug 1, 2015

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