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

Job.cancel() doesn't dispose SingleSource called via rx2 await() #1671

Closed
CarsonRedeye opened this issue Nov 25, 2019 · 6 comments
Closed

Job.cancel() doesn't dispose SingleSource called via rx2 await() #1671

CarsonRedeye opened this issue Nov 25, 2019 · 6 comments

Comments

@CarsonRedeye
Copy link

CarsonRedeye commented Nov 25, 2019

I have a presenter that implements CoroutineScope and uses a SupervisorJob as the scope job.

Within that presenter, I create a long running network request like this:

launch {
    withContext(Dispatchers.IO) {
        repo.downloadThing(thingId)
    }
}

Then I quickly cancel the job in the onDestroy function of my presenter

fun destroy() {
    supervisorJob.cancel()
}

My SingleSource isn't disposed:

suspend fun downloadThing(thingId: String) {
    service.download(thingId)
                .doOnDispose { Timber.d("DISPOSED") } // This is never called
                .doOnError { Timber.d("ERROR") } // This is called only after the network call completes (leaked)
                .await()
}

Using version 1.3.2
@CarsonRedeye CarsonRedeye changed the title Job.cancel doesn't dispose SingleSource called via rx2 await() Job.cancel() doesn't dispose SingleSource called via rx2 await() Nov 25, 2019
@qwwdfsad
Copy link
Member

Could you please provide a self-contained reproducer?
We have both code and tests ensuring that the source is properly disposed when its awaiter is cancelled.
E.g. the following smoke successfully prints "Disposed":

val awaiter = launch { //
    Single.never<Int>()
        .doOnDispose { println("Disposed") }
        .doOnError { println("Error") }
        .await()
}
yield()
awaiter.cancelAndJoin()

Thus either we have a sophisticated bug hidden in some complex interaction (and then we definitely need a reproducer to fix it) or it's just a bug in your testing/application code

@CarsonRedeye
Copy link
Author

CarsonRedeye commented Nov 25, 2019

I am trying to provide a re-producer but I'm not familiar with testing coroutines. For example I don't know what yield() does here. Would it be ok to provide a small android app example?

@qwwdfsad
Copy link
Member

Yes, android app should do the trick. Basically, anything that I can just clone & run will work

@CarsonRedeye
Copy link
Author

Ok so I've narrowed it down to happening only if Retrofit is used with Rxjava adapter. If it is used with suspend function it cancels correctly

@CarsonRedeye
Copy link
Author

CarsonRedeye commented Nov 28, 2019

Here is a reproduction app. I'm not sure if it is a retrofit problem or an rx2 await problem. The workaround is to call subscribeOn(Schedulers.io()) again on the single that is being called by rx2. https://drive.google.com/open?id=16kK23594rI_6Qa9tiOZBi69n8BEg6pGS

qwwdfsad added a commit that referenced this issue Nov 28, 2019
… integrate with APIs that may block the current thread, but react on cancellation

Fixes #1671
@qwwdfsad
Copy link
Member

qwwdfsad commented Nov 28, 2019

Thanks, that really helps!
Indeed, this is an interaction on the edge of coroutines cancellation and Rx API. The problem here is that getSomethingSlowly().subscribe() (transitively via await) blocks the current thread before dependency between cancellation of a coroutine and disposal of a subscription is established. Using subsribeOn with any scheduler apart from direct/trampoline should fix your problem.

I have a potential fix, but I am not sure whether we can include it in 1.3.3

qwwdfsad added a commit that referenced this issue Nov 28, 2019
… integrate with APIs that may block the current thread, but react on cancellation

Fixes #1671
qwwdfsad added a commit that referenced this issue Dec 2, 2019
… integrate with APIs that may block the current thread, but react on cancellation

Fixes #1671
qwwdfsad added a commit that referenced this issue Apr 27, 2020
… integrate with APIs that may block the current thread, but react on cancellation

Fixes #1671
recheej pushed a commit to recheej/kotlinx.coroutines that referenced this issue Dec 28, 2020
… integrate with APIs that may block the current thread, but react on cancellation (Kotlin#1680)

Fixes Kotlin#1671
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