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

IORunLoop loops indefinitely after unsafeRunSync is called #326

Closed
entangled90 opened this issue Aug 29, 2018 · 16 comments
Closed

IORunLoop loops indefinitely after unsafeRunSync is called #326

entangled90 opened this issue Aug 29, 2018 · 16 comments

Comments

@entangled90
Copy link

The following snippet of code after is being run continues to utilize 1 cpu (100% usage):

import cats.effect.IO
import cats.effect.internals.IOContextShift
import cats.implicits._
import scala.concurrent.duration._

import scala.concurrent.ExecutionContext
implicit val ec = IOContextShift.global

implicit val timer = IO.timer(ExecutionContext.Implicits.global)

val f = for {
  fiber <- IO("success").foreverM.start
  _     <- fiber.cancel
} yield "success"

IO.race(f, IO.sleep(200 millis) *> IO("failed")).unsafeRunSync() == Left("success")

As other have pointed out in gitter, IO("success") is not cancelable, but IO("success").foreverM should be.

I profiled the application (run inside sbt test) with yourkit and this is the relevant part:
schermata 2018-08-29 alle 13 32 39

@entangled90 entangled90 changed the title IORunLoop loops indefinitely after unsafeRunSync is called IORunLoop loops indefinitely after unsafeRunSync is called Aug 29, 2018
@SystemFw
Copy link
Contributor

SystemFw commented Aug 29, 2018

What version are you on? RC3 since you have ContextShift

@mpilquist
Copy link
Member

What if you write f this way instead?

IO("success").foreverM.start.bracket(_ => IO.pure("success"))(_.cancel)

@alexandru
Copy link
Member

alexandru commented Aug 29, 2018

What does foreverM do?

As other have pointed out in gitter, IO("success") is not cancelable, but IO("success").foreverM should be.

Not really, without any async or "cancel" boundaries inserted in such a loop, it isn't. I mean the background process that does the processing, not talking about the bind continuation of the result.

Note that that race will create a race condition and ignore the result of your loop afterwards, but that loop continues to run on another thread.

@SystemFw
Copy link
Contributor

Not really, without any async or "cancel" boundaries inserted in such a loop, it isn't.

Shouldn't it be with auto-cancellable binds?

@alexandru
Copy link
Member

Yes, but that flag is checked only on async boundaries (e.g. stuff built with IO.async).

@SystemFw
Copy link
Contributor

SystemFw commented Aug 29, 2018

Yes, but that flag is checked only on async boundaries (e.g. stuff built with IO.async).

That is very surprising. I would expect that snippet to work. Otherwise what is the difference with non-auto-cancellable-binds?

@SystemFw
Copy link
Contributor

SystemFw commented Aug 29, 2018

foreverM == fa.flatMap(_ => fa) (but stack-safe for all monads due to tailRecM)

@alexandru
Copy link
Member

alexandru commented Aug 29, 2018

Common sense in this case is not reliable. Most people expect magic in the case of cancelation.

Even if you take care of this case:

  1. it's useless; obviously doing never-ending loops without async boundaries in it is a bad idea and not just for cancelability reasons
  • being able to interrupt something involves fairness; in absence of fairness it's not reasonable to expect interruption
  1. this does not take care of actual while loops or other JVM-specific logic, for which you'd need a Thread.interrupt

We need to draw lines and the line was currently drawn at async boundaries that we manage.

@alexandru
Copy link
Member

Also, fix is easy:

  1. with near-zero overhead: (IO.cancelBoundary *> task).foreverM
  2. with forking: (IO.shift *> task).foreverM

@SystemFw
Copy link
Contributor

SystemFw commented Aug 29, 2018

it's useless; obviously doing never-ending loops without async boundaries in it is a bad idea and not just for cancelability reasons

I guess I'm unclear on what exactly constitutes an async boundary in this case. Having some Timer.sleep operation would make that cancellable? How about waiting on an MVar? I'm not interested in this exact code but in real-world scenarios that resemble it, and perhaps those work :)

being able to interrupt something involves fairness; in absence of fairness it's not reasonable to expect interruption

This is a good point

this does not care of actual while loops or other JVM-specific logic, for which you'd need a Thread.interrupt

I don't buy into this argument in general, use flatMap chains instead of while, it's a best effort thing

@SystemFw
Copy link
Contributor

Wouldn't it make sense to have cancelBoundary in a type class btw?

@SystemFw
Copy link
Contributor

SystemFw commented Aug 29, 2018

Having some Timer.sleep operation would make that cancellable? How about waiting on an MVar?

I'm actually expecting these to work, the snippet above can't really be made to work without fairness (e.g. Fibers being scheduled by the IO interpreter "manually" or with auto-shifting), since it never yields back control. I do think cancelBoundary should be polymorphic, but perhaps it can't be supported by every data type out there?

EDIT: well, without fairness it can be made to work with best-effort guarantees, but not anything stronger than that

@alexandru
Copy link
Member

alexandru commented Aug 29, 2018

I guess I'm unclear on what exactly constitutes an async boundary in this case.

Everything that gets created via IO.async, IO.asyncF, IO.cancelable or IO.bracket.

And obviously anything that involves a shift or cancelBoundary, which are created via IO.async themselves.

Having some Timer.sleep operation would make that cancellable?

Yes, that's cancelable.

How about waiting on an MVar?

Yes, that's cancelable.

Wouldn't it make sense to have cancelBoundary in a type class btw?

Maybe, I've gone back and forth on it myself. I don't think there are many cases in which it is needed — besides the never ending, synchronous loops which frankly I haven't seen much in actual code. But it does well in demos that calculate Fibonacci 🙂

If we decide on it, it will have to wait for 2.0 though. 1.0.0 is next Monday and this is not the kind of emergency that can delay it.

@entangled90
Copy link
Author

entangled90 commented Aug 29, 2018

@mpilquist your snippets has the same issues as mine.
Adding a cancel boundary or forking the process fixes it.
I Guess you can close it. I did not have a clear idea of how cancellation works. Thanks!

@SystemFw
Copy link
Contributor

@alexandru I actually agree with you 100%

I saw the snippet and I unconsciously mapped it to things that actually do have an async boundary, without realising that that's the key thing that makes this behaviour happen.

@Avasil
Copy link

Avasil commented Aug 29, 2018

Seems like this section is out of date and could use refactoring before release to avoid confusion

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

6 participants