fabiendevos / nanotasks

Extremely light way to execute code in the background on Android. Alternative to AsyncTask.
Other
387 stars 36 forks source link

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

Open quanturium opened 9 years ago

quanturium commented 9 years ago

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

Gryzor commented 9 years ago

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

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

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

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

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

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

fabiendevos commented 9 years ago

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

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

kabouzeid commented 8 years ago

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