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

Observables created using `concat` don't get canceled #468

Closed
igstan opened this Issue Nov 21, 2017 · 1 comment

Comments

Projects
None yet
2 participants
@igstan

igstan commented Nov 21, 2017

Consider the following snippet:

import scala.concurrent._, duration._
import monix.reactive._
import monix.execution._, Scheduler.Implicits.global

val a = Observable.now(1L)
val b = Observable.interval(1.second)
val c = a ++ b
val d = c.foreach(println)

d.cancel() // Doesn't cancel the "interval" observable.

I would have expected the d.cancel() call to cancel the right-hand observable of the ++ operation, but it doesn't.

This seems to be a problem with all observables created using concat or one of its incarnations.

It only happens with the observable on the right. The left one gets canceled:

import scala.concurrent._, duration._
import monix.reactive._
import monix.execution._, Scheduler.Implicits.global

val a = Observable.interval(1.second).map(_ => "1st")
val b = Observable.interval(1.second).map(_ => "2nd")
val c = a ++ b
val d = c.foreach(println)

d.cancel() // Works as expected.
@alexandru

This comment has been minimized.

Member

alexandru commented Nov 21, 2017

It is because of: stateRef.lazySet(WaitOnActiveChild) — see current commit.

The problem is that the logic is based on getAndSet. When you do a getAndSet, it's an optimistic update done with the knowledge that you can revert it.

However in this case we have a loophole related to cancellation. That instruction I mentioned above is "ordered" in relation with the subscriber and what happens in onNext / onComplete / onError, but not in regards to cancellation itself, which is completely concurrent. So the story is that when the above instruction happens, the current cancellation state gets deleted.

The fix closes this problem, but now I'm worried that we'll have performance regression due to the getAndSet being obviously more expensive than a lazySet. Shouldn't be dramatic though.

@alexandru alexandru added the bug label Jan 7, 2018

@alexandru alexandru added this to the 3.0.0 milestone Jan 7, 2018

alexandru added a commit to alexandru/monix that referenced this issue Jan 20, 2018

alexandru added a commit that referenced this issue Jan 20, 2018

alexandru added a commit to alexandru/monix that referenced this issue Jan 21, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment