Skip to content
This repository has been archived by the owner on Jul 13, 2020. It is now read-only.

New async #78

Closed
yanex opened this issue Aug 8, 2015 · 5 comments
Closed

New async #78

yanex opened this issue Aug 8, 2015 · 5 comments

Comments

@yanex
Copy link
Member

yanex commented Aug 8, 2015

Current async requires too much responsibility from the developer.

1. The main problem is that user should not call anything from the Activity but should be able to access all resolved widgets (and other data stored in activity properties). For example:

public class SomeActivity : Activity() {
    var button: Button = null!!

    fun doJob() {
        async {
            uiThread {
                // Here we hold SomeActivity to access button property
                button.setText("A")
            }
        }
    }
}

The problem can be solved by casting this to the particular Activity type. It is complicated, unsafe and doesn't work for everything except activities:

uiThread {
    (this as SomeActivity).button.setText("A")
}

2. The signature for uiThread is:

fun AnkoAsyncContext.uiThread(f: Context.() -> Unit)

So passing Context as a receiver makes things complex cause you have this@SomeActivity and this from the uiThread receiver, these two this have different types though they are both Context. Even if you don't want to call anything outside the lambda directly, you can do it unintentionally.

3. If user calls async in some Fragment class, he/she probably wants to do something with that Fragment instance, and the underlying Context, that uiThread receives (as a receiver! 👎 ) is completely useless in this case.

Solution:

public class AnkoAsyncContext<T>(val weakRef: WeakReference<T>)

public fun <T> AnkoAsyncContext<T>.uiThread(f: (T) -> Unit) {
    val ref = weakRef.get() ?: return
    if (ContextHelper.uiThread == Thread.currentThread()) {
        f(ref)
    } else {
        ContextHelper.handler.post { f(ref) }
    }
}

public fun <T: Activity> AnkoAsyncContext<T>.activityUiThread(f: (T) -> Unit) {
    val activity = weakRef.get() ?: return
    if (activity.isFinishing()) return
    activity.runOnUiThread { f(activity) }
}

public fun <T: Fragment> AnkoAsyncContext<T>.fragmentUiThread(f: (T) -> Unit) {
    val fragment = weakRef.get() ?: return
    if (fragment.isDetached()) return
    val activity = fragment.getActivity() ?: return
    activity.runOnUiThread { f(fragment) }
}

public fun <T> T.async(task: AnkoAsyncContext<T>.() -> Unit): Future<Unit> {
    val context = AnkoAsyncContext(WeakReference(this))
    return BackgroundExecutor.submit { context.task() }
}

private object ContextHelper {
    val handler = Handler(Looper.getMainLooper())
    val uiThread = Looper.getMainLooper().getThread()
}

Use cases:

public class SomeActivity : Activity() {
    var button: Button = null!!

    fun doJob() {
        async {
            uiThread { act ->
                // Will be called if the Activity instance still exists (not collected by GC)
                act.button.setText("A")
            }
            activityUiThread { act ->
                // Will not be called if the Activity was closed
                act.button.setText("A")
            }
        }
    }
}

public class MyFragment : Fragment() {
    var counter: Int = 0

    fun doJob() {
        async {
            fragmentUiThread { fragment ->
                // Will not be called if the fragment is detached
                fragment.counter++
            }
        }
    }
}

Affected behaviour:

  • This breaks existing code (though, fixing the broken code is not too hard and existing memory leak problems could be discovered).
  • Context.uiThread() still looks strange. The name is very short, and the function is likely to be called outside async DSL. Furthermore, the name clashes with the uiThread defined for AnkoAsyncContext. Probably it should be renamed to Context.runOnMainApplicationThread() or something.

Possible shortcomings:

  • async now is defined for Any?.

Additional info:

Anko IDEA plugin must show the warning when the lambda passed to async captures an underlying Activity/Fragment/whatever.

@cypressious
Copy link
Contributor

The signature of asyncResult

public fun <T> T.asyncResult(task: AnkoAsyncContext<T>.() -> T): Future<T>

doesn't seem to make any sense to me. Why should the type of the Future be the same as the receiver type? This way, all I can ever produce is of type Fragment, Activity and so on.

I think the signature should be

public fun <T, R> T.asyncResult(task: AnkoAsyncContext<T>.() -> R): Future<R>

@cypressious cypressious mentioned this issue Sep 17, 2015
@yanex
Copy link
Member Author

yanex commented Sep 17, 2015

Proposed fix:

public fun <T, R> T.asyncResult(task: AnkoAsyncContext<T>.() -> R): Future<R> {
    val context = AnkoAsyncContext(WeakReference(this))
    return BackgroundExecutor.submit { context.task() }
}

public fun <T, R> T.asyncResult(executorService: ExecutorService, task: AnkoAsyncContext<T>.() -> R): Future<R> {
    val context = AnkoAsyncContext(WeakReference(this))
    return executorService.submit<R> { context.task() }
}

@cypressious
Copy link
Contributor

Looks fine!

@yanex yanex added this to the Anko 0.9 milestone Mar 25, 2016
@yanex yanex added the IDE label Mar 25, 2016
@yanex
Copy link
Member Author

yanex commented Mar 25, 2016

So the only thing that should be done is a warning in IDE:

Anko IDEA plugin must show the warning when the lambda passed to async captures an underlying Activity/Fragment/whatever.

@yanex yanex modified the milestones: Anko 0.9.1, Anko 0.9 May 18, 2016
@yanex yanex removed the open label May 3, 2017
@yanex
Copy link
Member Author

yanex commented May 3, 2017

kotlinx.coroutines is the recommended way to start the background tasks since Kotlin 1.1.
There's no need in adding the IDE support to old async() functions.

@yanex yanex closed this as completed May 3, 2017
@yanex yanex added the fixed label May 3, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

2 participants