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 AsyncTask to IntentService #969

Open
wants to merge 6 commits into
base: develop
from

Conversation

Projects
None yet
5 participants
@yulin2

yulin2 commented May 3, 2015

Hey owncloud-android 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 describe 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.

For example, in LogHistoryActivity, even if you use WeakReference for a text view in the AsyncTask,
you still hold a strong reference to the activity itself, which can lead to the problems described above. (I also opened a related pr before #892)

I refactored three AsyncTasks in owncloud-android to IntentService, with the help of a refactoring tool we developed. Do you think using IntentService is better in these three activities, and merge this pr?

@yulin2

This comment has been minimized.

Show comment
Hide comment
@yulin2

yulin2 May 15, 2015

I sent this pr several days ago. Any comments/suggestions on the changes?

yulin2 commented May 15, 2015

I sent this pr several days ago. Any comments/suggestions on the changes?

@masensio

This comment has been minimized.

Show comment
Hide comment
@masensio

masensio Sep 10, 2015

Hi @yulin2,

Thanks for your contribution.

Is it your first contribution in the OwnCloud Project? I've been searching your nickname in the developers list and I have not found it. If you haven't signed the Contributor Agreement, please send it ( https://owncloud.org/contribute/agreement/) to @karlitschek. We can't merged your PR without this agreement.

On the other hand, could you update your branch with the last version of the code? It is better to do the code review easily.

A question, why did you select IntentService instead of AsyncTaskLoader as in your PR #892?

Thanks again

masensio commented Sep 10, 2015

Hi @yulin2,

Thanks for your contribution.

Is it your first contribution in the OwnCloud Project? I've been searching your nickname in the developers list and I have not found it. If you haven't signed the Contributor Agreement, please send it ( https://owncloud.org/contribute/agreement/) to @karlitschek. We can't merged your PR without this agreement.

On the other hand, could you update your branch with the last version of the code? It is better to do the code review easily.

A question, why did you select IntentService instead of AsyncTaskLoader as in your PR #892?

Thanks again

@davivel davivel added the 0 - Backlog label Sep 10, 2015

@davivel

This comment has been minimized.

Show comment
Hide comment
@davivel

davivel Sep 10, 2015

Member

Need to think a bit about this. Not sure that replacing AsyncTasks with IntentService as a rule is fine for any case.

At first sight, I see a possible problem with returning results through LocalBroadcastManager. If I recall correctly, these broadcast messages cannot be made sticky, so could be lost by the listener Activity during rotations.

Besides, we'd like reduce the number of Services in the app. Right now we have too many for operations that could be merged in a single one. This seems to go in the other way. But this should not be a decision criteria for itself.

Member

davivel commented Sep 10, 2015

Need to think a bit about this. Not sure that replacing AsyncTasks with IntentService as a rule is fine for any case.

At first sight, I see a possible problem with returning results through LocalBroadcastManager. If I recall correctly, these broadcast messages cannot be made sticky, so could be lost by the listener Activity during rotations.

Besides, we'd like reduce the number of Services in the app. Right now we have too many for operations that could be merged in a single one. This seems to go in the other way. But this should not be a decision criteria for itself.

@davivel davivel added the technical label Sep 10, 2015

@rperezb rperezb added this to the backlog milestone Oct 15, 2015

@rperezb rperezb removed the 0 - Backlog label Oct 15, 2015

@owncloud-bot

This comment has been minimized.

Show comment
Hide comment
@owncloud-bot

owncloud-bot Feb 22, 2016

Contributor

Thanks a lot for your contribution!
Contributions to the core repo require a signed contributors agreement http://owncloud.org/about/contributor-agreement/

Alternatively you can add a comment here where you state that this contribution is MIT licensed.

Some more details about out pull request workflow can be found here: http://owncloud.org/code-reviews-on-github/

Contributor

owncloud-bot commented Feb 22, 2016

Thanks a lot for your contribution!
Contributions to the core repo require a signed contributors agreement http://owncloud.org/about/contributor-agreement/

Alternatively you can add a comment here where you state that this contribution is MIT licensed.

Some more details about out pull request workflow can be found here: http://owncloud.org/code-reviews-on-github/

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