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

Request: offer Job creation that will allow us to have the option on it : cancel, and cancel-with-interruption #2509

Closed
AndroidDeveloperLB opened this issue Feb 1, 2021 · 10 comments

Comments

@AndroidDeveloperLB
Copy link

Currently, either you use runInterruptible or you don't.
If you use it, cancel will interrupt.
If you don't, cancel will not interrupt.
The creation of the instance forces cancel to work in one way or the other. You have no choice after it's created.

On AsyncTask (here), you have a choice for every instance. cancel(true) will interrupt, and cancel(false) will not .
The creation of the instance doesn't force you to choose how it will be canceled.
You can choose to cancel with interruption in some case, and without in another.

@qwwdfsad
Copy link
Member

Thanks for the request.

We've considered such API (#1934), but decided to give up on it: thread interruption is not-so-widespread and mostly used wrongly. Also, such a design, unfortunately, is too intrusive and requires special InterruptedException handing over the codebase, so we've rejected it.

@AndroidDeveloperLB
Copy link
Author

@qwwdfsad But Kotlin is supposed to work with Java code fine, and on Java it's the way to stop long running operations.
What would you do if some function created multiple threads and waits for them?
This is the only way to signal the main one that we want to stop them.
Why wouldn't you want to support both ways? What's wrong here exactly?
You can't just force all code in the world to switch to Kotlin.

@qwwdfsad
Copy link
Member

qwwdfsad commented Feb 20, 2021

For usages of blocking code, it is recommended to use runInterruptible.
Due to its nature, blocking code already has to be treated explicitly in suspending functions and coroutines.
And if one is already have to use a custom dispatcher if they want to block in suspend function, then If they also want to support interruptions, it's enough to swap withContext(Dispatchers.IO) to runInterruptible(Dispatchers.IO)

@AndroidDeveloperLB
Copy link
Author

@qwwdfsad Not sure I understand. You mean to say that it is possible to choose how to cancel a given code, with interruption and without, after the task was created?
If so, can you please demonstrate? Suppose I give you a normal Java function "foo", and you want to create a function that uses it, yet it also allows to be canceled using coroutines, how would you do this?

@qwwdfsad
Copy link
Member

suspend fun fooUser(shouldInterruptIfCancelled: Boolean) {
    if (shouldInterruptIfCancelled) {
        runInterruptible { foo() }
        return 
    }
    foo()
}

@AndroidDeveloperLB
Copy link
Author

@qwwdfsad I don't understand. How when using this, you will then be able to choose to cancel it with interruption and without?
What you wrote seems to be the exact issue I'm describing : That you can't have them both. You have to choose one of them and stick with it.
You either use runInterruptible or you don't. Use runInterruptible , and you can interrupt but can't cancel. Don't use, and you can't interrupt, but you can cancel. You can't have both.

@qwwdfsad
Copy link
Member

Got it, thanks for the clarification.

Could you please elaborate on the use-case of such an API? Maybe the use-case will shed some light on other potential solutions that are less intrusive and can be introduced to the JVM-only module

@AndroidDeveloperLB
Copy link
Author

@qwwdfsad Yes, sure. I wrote the use case before, so I will explain again:
I want to be able to choose how to cancel the task. Using thread-interrupt or not.
Just like I could before, using AsyncTask.
AsyncTask is deprecated, and coroutines is one of the things that were suggested to use instead.
Deprecation means there is something that's supposed to be better, so you should be able to do everything you could do before, otherwise it would be impossible to migrate.

@qwwdfsad
Copy link
Member

Could you please explain the actual domain-specific problem? E.g. why it is important to be able to dynamically choose cancellation mode and why the problem couldn't be solved otherwise (e.g. via runInterruptible) or would require much more code.

Becuase when AsynkTask was deprecated, there wasn't any demand or actual use-cases of such a flexibility

@AndroidDeveloperLB
Copy link
Author

@qwwdfsad
It is important when recommending a better replacement, to show that the previous one can be fully replaced.
Why do you think that AsyncTask is more flexible? It's based on very basic Threads mechanisms, and so should coroutines. I'm pretty sure coroutines uses thread pools classes the same way.

This reminds me of a very similar case. I was told that the perfect replacement for IntentService (which was deprecated) is WorkManager. I tried what I can to migrate to it, but then I noticed a very weird thing: all app-widgets got updated more often than usual. I was sure I did something wrong and spent hours on finding out what's going on. Was sure that I didn't implement WorkManager or that I ruined something with App-widget.
Turned out that WorkManager has a terrible flaw that causes AppWidgets to update themselves for no good reason:
https://www.reddit.com/r/android_devs/comments/llq2mw/question_why_should_it_be_expected_that/

And Google, instead of admitting this is a terrible implementation that didn't occur before and didn't need a special care, closed the issue with saying that you should use a ridiculous workaround instead: Always have a pending work for WorkManager.
No matter how many people claimed this is a bug (including CommonsWare, here), it didn't matter:
https://issuetracker.google.com/issues/115575872

So, this should show you a good lesson of how it's best to offer migrations. Migrations should have the minimal issues possible. Should avoid workarounds. Should cover as much as possible from the original (otherwise why switch?).

Google works today a lot on Compose. How would you think people would react if in the end it won't have a very basic thing they were used to (like TextView) , and Google will just say to use some workaround instead (like "it's ok, draw your own text") ?

That's how basic it is here. Canceling a task the way you want. That's very basic.
I could easily do this with Thread class (a field of "isCancelled" and a reference to the Thread instance to call "interrupt()" on it). Why shouldn't I be able to do this with coroutines?

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

2 participants