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.

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

Sample shows wrong way to create the Task by instantiating anonymous classes #1

Open
quanturium opened this issue Feb 3, 2015 · 9 comments

Comments

@quanturium
Copy link

Sample shows wrong way to create a BackgroundTask by instantiating anonymous classes and therefore keeping a reference to the activity / leaking it.

@Gryzor
Copy link

Gryzor commented Feb 4, 2015

How so? If it uses a WeakReference?

The code in the sample does:

Tasks.executeInBackground(v.getContext(), new BackgroundWork<String>() {

Where v is the view. If the Activity stops and destroys everything, NanoTask won't prevent that GC because the reference to that context is Weak.

Maybe you're seeing something I am not? :)

@quanturium
Copy link
Author

The WeakReference is good, the problem is with the anonymous classes: new BackgroundWork() and new Completion() are anonymous classes which mean they hold a strong reference to the outer class. The Task object holds a strong reference to those anonymous classes. In other words: Thread -> Task -> BackgroundWork/Completion -> Context. Hope it makes sense.

@Gryzor
Copy link

Gryzor commented Feb 5, 2015

Oh I understand what you say and I agree. But the truth is, there's no easy way to avoid that. The way I would do it is by not having that context there, because I think it's the developer's responsibility to check for the context, but if you do that, then you don't need nanotasks :-)

update: After looking at the source code once again (vs. the sample) I don't think he's leaking. There's no place in the code where the context is hard referenced. Even if the anonymous BackgroundWork retains a reference to the activity, the context itself is destroyed (null) if the activity dies.

I don't think there's a leak here.

The Task contains references to BackgroundWork/Completion, but these contain no reference to the Activity or Context whatsoever. This is contained in the task itself (as weak reference). So when the activity is destroyed, that weak reference is not stopping the context from being destroyed and GCed. The task just will try to access a null context (which, in turn, will be catch by the if condition).

@almozavr
Copy link

Every anonymous class instance holds strong ref to parent class which it's created in. So, no matter context is passed or not as a weak ref, background work and completion will hold the ref to parent class (e.g. activity) till their completion.

@sergeyzenchenko
Copy link

This library is useless. It's not solving any problems. Captured weak context will not be deleted because it still captured by anonymous callback classes (if we are talking about creating callbacks from Activity).

@ChrisMCMine
Copy link

@sergeyzenchenko so true, it's just an asynctask wrapper at the moment

@fabiendevos
Copy link
Owner

@sergeyzenchenko @ChrisMCMine etc.
You're totally right. It was also initially conceived to be purely an asynctask wrapper and just improve the asynctask API (I didn't want to have to subclass, and I wante onSuccess/onError callbacks).

I'm working on solving this issue. One option is to clean-up the listeners with a LifeCycleCallback. This is a bit tricky to implement right though. I should have something ready soon. This is probably going to be NanoTask v2.

Thanks for the feedback.

@kamilwlf
Copy link

When you plan to release a new version? NanoTask v2?

@kabouzeid
Copy link

Whats the problem in holding a weak reference to the completion and backgroundWork objects too? This would solve the leaking issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

8 participants