Fix ExplicitlyTriggeredScheduler cancellation

[bszwej]

Fixes #28604

This also needs to be backported to 2.5.x (PR for 2.5.x: #28618)

[lightbend-cla-validator]

Hi @bszwej,

Thank you for your contribution! We really value the time you've taken to put this together.

Before we proceed with reviewing this pull request, please sign the Lightbend Contributors License Agreement:

http://www.lightbend.com/contribute/cla

[akka-ci]

Thank you for your pull request! After a quick sanity check one of the team will reply with 'OK TO TEST' to kick off our automated validation on Jenkins. This compiles the project, runs the tests, and checks for things like binary compatibility and source code formatting. When two team members have also manually reviewed and (perhaps after asking for some amendments) accepted your contribution, it should be good to be merged.

For more details about our contributing process, check out CONTRIBUTING.md - and feel free to ask!

[bszwej]

Before applying the fix, this test was failing.

[bszwej]

Before we were updating the task with new execution time. Because of this, the Cancellable was left with stale reference (this line: https://github.com/akka/akka/pull/28614/files#diff-7a1a738a6673f4092734cfa193957d7dR109), and hence the task could not be canceled (=removed from the scheduled map).

[akka-ci]

Test FAILed.

[akka-ci]

Test FAILed.

[raboof]

Nice, one nitpick about the class name of the test but otherwise LGTM!

[raboof]

ah, it seems this needs a sbt akka-testkit/scalafmt

[akka-ci]

Test FAILed.

[bszwej]

Thank you for the review @raboof 👍
I introduced the changes. validatePullRequest now passes.

On Jenkins it failed for some other reason. How can we rerun it?

Also, can we backport it to 2.5.x according to: https://github.com/akka/akka/blob/master/CONTRIBUTING.md#backporting? Should me, or you, create a PR for the backport?

[raboof]

PLS BUILD

[raboof]

Should me, or you, create a PR for the backport?

Would be great if you can help out with that as well! Otherwise we'll take care of it (probably somewhere next week).

[akka-ci]

Test FAILed.

[bszwej]

Would be great if you can help out with that as well! Otherwise we'll take care of it (probably somewhere next week).

I prepared a PR for 2.5.x: #28618

[akka-ci]

Test FAILed.

[bszwej]

I just found one more problem with the new implementation. Since we're storing Items as keys in the map (Item(interval: Option[FiniteDuration], runnable: Runnable)), the following test would not pass:

    "schedule many tasks executed once" in new TestScope {
      scheduler.scheduleOnce(1.seconds, runnable)(system.dispatcher)
      scheduler.scheduleOnce(1.seconds, runnable)(system.dispatcher)
      scheduler.timePasses(2.seconds)

      counter.get() shouldBe 2 // fails, because counter.get() is 1
    }

since it'd try to insert 2 duplicated entries into the map.

Let's refrain from merging this PR now. I'll come with a better solution in the following days.

[akka-ci]

Test FAILed.

[akka-ci]

Test PASSed.

[bszwej]

I added uniqueness to Item, because otherwise, the following test would fail:

    "schedule many tasks executed once" in new TestScope {
      scheduler.scheduleOnce(1.seconds, runnable)(system.dispatcher)
      scheduler.scheduleOnce(1.seconds, runnable)(system.dispatcher)
      scheduler.timePasses(2.seconds)

      counter.get() shouldBe 2 // fails, because counter.get() is 1
    }

[bszwej]

@raboof I introduced the final changes in this PR, and also in the backport (#28618). Feel free to look again and/or merge.

[raboof]

Looking very good - perhaps we could avoid needing the UUID by making Item a non-case class?

[raboof]

Nice! LGTM!

[akka-ci]

Test PASSed.