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

replace-flatmap-to-guaranteeCase #934

Conversation

@BambooTuna
Copy link
Contributor

commented Jun 27, 2019

fixes #915

@Avasil

This comment has been minimized.

Copy link
Collaborator

commented Jun 27, 2019

@alexandru The following test fails:

  test("Task.wanderUnordered should log errors if multiple errors happen") { implicit s =>
    implicit val opts = Task.defaultOptions.disableAutoCancelableRunLoops

    val ex = DummyException("dummy1")
    var errorsThrow = 0
    val gather = Task.wanderUnordered(Seq(0, 0)) { _ =>
      Task.raiseError[Int](ex).executeAsync.doOnFinish { x =>
        if (x.isDefined) errorsThrow += 1; Task.unit
      }
    }

    val result = gather.runToFutureOpt
    s.tick()

    assertEquals(result.value, Some(Failure(ex)))
    assertEquals(s.state.lastReportedError, ex)
    assertEquals(errorsThrow, 2)
  }

because with guaranteeCase it gets to the ExitCase.Canceled even with autoCancelableRunLoops disabled. Is it intended?

@BambooTuna

This comment has been minimized.

Copy link
Contributor Author

commented Jun 29, 2019

If there is a sample of how autoCancelableRunLoops behave, can you tell me?

@BambooTuna

This comment has been minimized.

Copy link
Contributor Author

commented Jun 29, 2019

@alexandru The following test fails:

  test("Task.wanderUnordered should log errors if multiple errors happen") { implicit s =>
    implicit val opts = Task.defaultOptions.disableAutoCancelableRunLoops

    val ex = DummyException("dummy1")
    var errorsThrow = 0
    val gather = Task.wanderUnordered(Seq(0, 0)) { _ =>
      Task.raiseError[Int](ex).executeAsync.doOnFinish { x =>
        if (x.isDefined) errorsThrow += 1; Task.unit
      }
    }

    val result = gather.runToFutureOpt
    s.tick()

    assertEquals(result.value, Some(Failure(ex)))
    assertEquals(s.state.lastReportedError, ex)
    assertEquals(errorsThrow, 2)
  }

because with guaranteeCase it gets to the ExitCase.Canceled even with autoCancelableRunLoops disabled. Is it intended?

@alexandru The following test fails:

  test("Task.wanderUnordered should log errors if multiple errors happen") { implicit s =>
    implicit val opts = Task.defaultOptions.disableAutoCancelableRunLoops

    val ex = DummyException("dummy1")
    var errorsThrow = 0
    val gather = Task.wanderUnordered(Seq(0, 0)) { _ =>
      Task.raiseError[Int](ex).executeAsync.doOnFinish { x =>
        if (x.isDefined) errorsThrow += 1; Task.unit
      }
    }

    val result = gather.runToFutureOpt
    s.tick()

    assertEquals(result.value, Some(Failure(ex)))
    assertEquals(s.state.lastReportedError, ex)
    assertEquals(errorsThrow, 2)
  }

because with guaranteeCase it gets to the ExitCase.Canceled even with autoCancelableRunLoops disabled. Is it intended?

Enabling AutoCancelableRunLoops cancels the processing after the task where the error occurred.

I want to continue processing even if an error occurs in this test, but because the processing is canceled on the way, the test did not pass.

Is this forecast correct?

@Avasil

This comment has been minimized.

Copy link
Collaborator

commented Jun 29, 2019

If there is a sample of how autoCancelableRunLoops behave, can you tell me?

It is a flag which tells whether we should check for cancelation status after asynchronous boundary

I want to continue processing even if an error occurs in this test, but because the processing is canceled on the way, the test did not pass.

Is this forecast correct?

Pretty much. I think if you add .uncancelable in the failing tests, e.g.:

val gather = Task.wanderUnordered(Seq(0, 0)) { _ =>
  Task.raiseError[Int](ex).executeAsync.doOnFinish { x =>
    if (x.isDefined) errorsThrow += 1; Task.unit
  }.uncancelable
}

they will pass with your implementation. I think the task actually finishes before cancelation but is still considered canceled so it is ExitCase.Canceled. In previous implementation if we call .doOnFinish(...).doOnCancel(...) both of these will execute... So it has different behavior than guaranteeWith. What would you expect @oleg-py @alexandru ?

@Avasil

This comment has been minimized.

Copy link
Collaborator

commented Jun 29, 2019

BTW @BambooTuna Could you also remove DoOnFinish class (it's here: https://github.com/monix/monix/blob/master/monix-eval/shared/src/main/scala/monix/eval/Task.scala#L4482) - it won't be necessary if we change it. :)

You will have to add exclude[MissingClassProblem]("monix.eval.Task$DoOnFinish")
here: https://github.com/monix/monix/blob/master/project/MimaFilters.scala#L6 because it will break Binary Compatibility

@BambooTuna

This comment has been minimized.

Copy link
Contributor Author

commented Jun 29, 2019

To @Avasil
Sorry I'm late.

What I added

Question

In the test, I get an error of

monix.reactive.internal.operators.MapParallelUnorderedSuite
- MVar (async) — issue #380: with foreverM; with latch *** FAILED ***
  AssertionException: ; timed-out after 10 seconds (MVarJVMSuite.scala:318)
    minitest.api.Asserts.assert(Asserts.scala:38)
    minitest.api.Asserts.assert$(Asserts.scala:34)
    monix.catnap.BaseMVarJVMSuite.assert(MVarJVMSuite.scala:100)
    monix.catnap.BaseMVarJVMSuite.$anonfun$new$94(MVarJVMSuite.scala:318)
    scala.collection.immutable.Range.foreach$mVc$sp(Range.scala:158)
    monix.catnap.BaseMVarJVMSuite.$anonfun$new$93(MVarJVMSuite.scala:303)
    minitest.TestSuite.$anonfun$test$1(TestSuite.scala:33)
    minitest.api.TestSpec$.$anonfun$sync$1(TestSpec.scala:51)
    minitest.api.TestSpec.apply(TestSpec.scala:27)
    minitest.api.Properties.$anonfun$iterator$2(Properties.scala:38)
    minitest.api.TestSpec.apply(TestSpec.scala:27)
    minitest.runner.Task.loop$1(Task.scala:56)
    minitest.runner.Task.$anonfun$execute$1(Task.scala:63)
    scala.concurrent.Future.$anonfun$flatMap$1(Future.scala:307)
    scala.concurrent.impl.Promise.$anonfun$transformWith$1(Promise.scala:41)
    scala.concurrent.impl.CallbackRunnable.run(Promise.scala:64)
    java.util.concurrent.ForkJoinTask$RunnableExecuteAction.exec(ForkJoinTask.java:1402)
    java.util.concurrent.ForkJoinTask.doExec(ForkJoinTask.java:289)
    java.util.concurrent.ForkJoinPool$WorkQueue.runTask(ForkJoinPool.java:1056)
    java.util.concurrent.ForkJoinPool.runWorker(ForkJoinPool.java:1692)
    java.util.concurrent.ForkJoinWorkerThread.run(ForkJoinWorkerThread.java:157)

, in my local environment.
So, I tried to increase the timeout time, and the test succeeded

https://github.com/BambooTuna/monix/blob/feature/replace-flatMap-to-guaranteeCase-at-Task.doOnFinish/monix-catnap/jvm/src/test/scala/monix/catnap/MVarJVMSuite.scala#L121
To

val timeout = if (isCI) 30.seconds else 30.seconds

I don't know if i have to change it.

Other

val gather = Task.wanderUnordered(Seq(0, 0)) { _ =>
  Task.raiseError[Int](ex).executeAsync.doOnFinish { x =>
    if (x.isDefined) errorsThrow += 1; Task.unit
  }.uncancelable
}

Thanks for the sample code!
I just thought that uncancelable should be added here.

final def doOnFinish(f: Option[Throwable] => Task[Unit]): Task[A] =
  this.uncancelable.guaranteeCase {
    case ExitCase.Completed => f(None)
    case ExitCase.Canceled => Task.unit
    case ExitCase.Error(e) => f(Some(e))
  }
exclude[IncompatibleMethTypeProblem]("monix.eval.internal.TaskRunLoop.popNextBind")
exclude[IncompatibleMethTypeProblem]("monix.eval.internal.TaskRunLoop.popNextBind"),
// Breaking changes for https://github.com/monix/monix/pull/934
exclude[MissingClassProblem]("monix.eval.Task$DoOnFinish")

This comment has been minimized.

Copy link
@Avasil

Avasil Jun 29, 2019

Collaborator

There are two lists, this one is older, try putting it at the top in the first Seq

You can verify it with mimaReportBinaryIssues

@Avasil

This comment has been minimized.

Copy link
Collaborator

commented Jun 29, 2019

Sorry I'm late.

Take your time and thank you for the interest in contributing!

I don't know if i have to change it.

I don't recall any problems with it so let's see if it fails on CI

Thanks for the sample code!
I just thought that uncancelable should be added here.

The issue was possibly wrong test (unless I'm wrong here but in that case, guaranteeWith might be buggy), doOnFinish should still work with cancelable Task. In the test, it was canceled because if any of the tasks in Task.wander and Task.gather fails, the others are canceled.

@BambooTuna

This comment has been minimized.

Copy link
Contributor Author

commented Jun 30, 2019

Tests pass in a local environment, but it is useless in CI. . .

@Avasil

This comment has been minimized.

Copy link
Collaborator

commented Jun 30, 2019

I think tests arent formatted, try test:scalafmt or scalafmtAll

@BambooTuna

This comment has been minimized.

Copy link
Contributor Author

commented Jun 30, 2019

@Avasil
The test passed successfully!
Thank you very much for your support, and I would like to put out a PR if there is something I can do in the future!

@Avasil

This comment has been minimized.

Copy link
Collaborator

commented Jun 30, 2019

Thank you @BambooTuna ! That's awesome.

I'm going back to Europe soon. I'll wait until I come back to merge PR in case other maintainers have thoughts about the issue with earlier version of the test.

@Avasil Avasil merged commit 29a0caf into monix:master Jul 1, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.