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

Cancel should always wait for the finalizer #1120

Merged
merged 7 commits into from Feb 29, 2020

Conversation

Avasil
Copy link
Collaborator

@Avasil Avasil commented Feb 3, 2020

Port of my change in typelevel/cats-effect#727 but we also support tryReactivate to implement onCancelRaiseError. Prior to the PR we backpressured finalizers only for the first action

I'm not sure how to handle def tryReactivate(): Boolean, I did something that might work but any suggestions welcome. It doesn't wait for finalizers BUT the method is private and only exists for onCancelRaiseError in conn.cancel.map(_ => ...) so it shouldn't be an issue.

Fixes #1086 and fixes #1124

@Avasil Avasil requested review from alexandru and oleg-py Feb 3, 2020
oleg-py
oleg-py approved these changes Feb 4, 2020
Copy link
Collaborator

@oleg-py oleg-py left a comment

Ok, I have very little understanding of what it actually does, but the tests look reasonable :)

@Avasil
Copy link
Collaborator Author

Avasil commented Feb 4, 2020

Ok, I have very little understanding of what it actually does, but the tests look reasonable :)

My understanding of finalizers implementation goes like this:

  1. If it's a success or a failure then finalizers goes after flatMap/errorHandle like a normal Task
  2. Finalizers are added to the TaskConnection stack so if Task is canceled then they are invoked and backpressured via TaskConnection#cancel
  3. Whatever causes finalizers to run, it marks Atomic Reference that the task is no longer waiting for any results and finalizers were executed

The problem was that after setting Atomic Reference any next cancel would become a no-op even if finalizers are still in progress so I just added a Promise there to wait for it.

I added ForwardCancelable from cats.effect.IO implementation because I think there was an issue that a Task could be canceled before a finalizer was installed

Sorry for a chaotic explanation but I hope it adds a bit more context :D
My hope is that @alexandru could find some time to review it.
I already went through review in cats-effect but it would give me more confidence

@alexandru
Copy link
Member

alexandru commented Feb 14, 2020

Would like to do a review — please allow me until Monday, or if I go silent again, then proceed without me 😀

@Avasil
Copy link
Collaborator Author

Avasil commented Feb 14, 2020

Awesome @alexandru I will really appreciate it! During my work on monix-bio I became more confident with internals but it's hard to find anyone to do proper review and confirm my understanding :D

I'm about to update PR because I also recognized an issue with our parallel operators.
We currently do:

conn.cancel.runAsyncAndForget
cb.onError(ex)

which causes some problems, e.g.

import monix.execution.Scheduler.Implicits.global

Task.race(
  Task.sleep(1.seconds).bracket(_ => Task.unit)(_ => Task { println("release") }),
  Task.sleep(100.millis)
).flatMap(_ => Task(println("end"))).runSyncUnsafe()

will exit the program without waiting for release.
cats.effect.IO waits for release here so the change will be consistent

@Avasil Avasil added this to the 3.2.0 milestone Feb 18, 2020
@Avasil
Copy link
Collaborator Author

Avasil commented Feb 29, 2020

@alexandru I am merging it but if you find time to go through it then we could just do a follow up PR :D

@Avasil Avasil merged commit 14351b7 into monix:master Feb 29, 2020
1 check passed
@Avasil Avasil deleted the bracket-wait-finalizers branch Mar 2, 2020
mdedetrich pushed a commit to mdedetrich/monix that referenced this issue Mar 28, 2020
* Cancel should always wait for the finalizer

* Remove leftover comments

* Fix tryReactivate

* add extra test

* Backpressure on finalizers in parallel operators

* Suspend p.success() outside CAS loop

* Update TaskConnection.scala
@ppressives ppressives mentioned this pull request Jun 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants