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

Bring back IO#runAsync and friends? #2530

Closed
matil019 opened this issue Nov 11, 2021 · 39 comments · Fixed by #2613
Closed

Bring back IO#runAsync and friends? #2530

matil019 opened this issue Nov 11, 2021 · 39 comments · Fixed by #2613

Comments

@matil019
Copy link

Nice to meet you!

I'm having a difficulty using Cats Effect 3 on Scala.js due to the change of the thread model.

Some JS functions such as Event.preventDefault() have to be run synchronously or won't have any effect. In CE2, we can just use IO because it is run synchronously until a first async boundary. For example, if we wanted to implement some click event handler:

val more: IO[Unit] = ???
// OK in CE2, bad in CE3
def onClick(e: org.scalajs.dom.Event): IO[Unit] =
  IO{ e.preventDefault() } *> more

In CE3, we must use SyncIO without converting it into an IO. Unless I'm missing something, we have to resort to IO#unsafeRunAndForget:

val more: IO[Unit] = ???
// bad
def onClick(e: org.scalajs.dom.Event): SyncIO[Unit] =
  SyncIO{ e.preventDefault() }.to[IO] *> more
// OK
def onClick(e: org.scalajs.dom.Event): SyncIO[Unit] =
  SyncIO{ e.preventDefault(); more.unsafeRunAndForget() }
// OK
def onClick(e: org.scalajs.dom.Event): SyncIO[Unit] =
  SyncIO{ e.preventDefault() } *> SyncIO{ more.unsafeRunAndForget() }

The story is complicated because often more in the above example must be asynchronous. For example, it can be an AJAX call triggered by clicking a link. I guess this is going to be a common pattern in my project, and I definitely wouldn't like to sprinkle unsafeRunAndForget all over it.

If there were IO#runAsync like in CE2, the last code could be rewritten like this:

def onClick(e: org.scalajs.dom.Event): SyncIO[Unit] =
  SyncIO{ e.preventDefault() } *> more.runAsync(_ => IO.unit)

(I'd like to point out IO#runAsync would be equivalent to runAff_ in purescript-aff, and IO#runAndForget would correspond to launchAff_.)

The migration guide refers to Dispatcher as the replacement of IO#runAsync but it doesn't have any methods that return SyncIO[_]. I think it is reasonable because Dispatcher shouldn't be coupled to SyncIO, but it may be helpful to have IO, as a concrete effect type, have a conversion from IO to SyncIO.

@vasilmkd
Copy link
Member

Hi 👋! Thanks for raising this issue.

Have you tried using the IO#syncStep method? It converts an IO to a SyncIO which when unsafeRunSync() will run the side effects of the IO until completion or until the first asynchronous boundary, if running the IO to completion isn't possible (imagine that this method can execute all operations which are shared by both IO and SyncIO). This method was introduced to address very similar concerns from Scala.js users.

Do report if this method is helpful, otherwise we can look for a new feature request.

@matil019
Copy link
Author

@vasilmkd Yes, I did try IO#syncStep but it doesn't solve the problem. In my use case, I have to run IO to its end no matter what. If I understand correctly, when an IO has any async boundaries, IO#syncStep returns a Left whose content is another IO, and I have to run it again (possibly with unsafeRunAndForget) or use unsafeRunHere designed in #2478 .

But it doesn't matter much; running an IO has to be wrapped in SyncIO to keep the entire code pure. Also, the result type of SyncIO must be Unit so something like SyncIO[IO[Unit]] is unacceptable.

I'm using scalajs-react, which supports SyncIO and IO as synchronous and asynchronous effects, respectively. I don't have a control over how IOs are run in general; I don't think I can replace the ultimate calls to IO#unsafeRunAsync with IO#unsafeRunHere. And even if I could, I wouldn't like to do it because it would affect the global behavior of synchronous-ness of IO effects.

@armanbilge
Copy link
Member

armanbilge commented Nov 11, 2021

@matil019 I think @vasilmkd is right, actually syncStep is exactly what you need :)

If I understand correctly, when an IO has any async boundaries, IO#syncStep returns a Left whose content is another IO, and I have to run it again (possibly with unsafeRunAndForget) or use unsafeRunHere designed in #2478 .

That's right, except the IO will run as if it's a SyncIO until it hits the first async boundary. So if you have an IO:

IO(step1()) *> IO(step2()) *> IO(step3()).start

using syncStep will run steps 1 and 2 synchronously, and return a Left containing an IO for step 3. Then, you will need to use unsafeRunAndForget to complete the IO. Have you tried syncStep in your code?

Personally I think that having to do these two steps is cumbersome which is why I proposed #2478.

@vasilmkd
Copy link
Member

vasilmkd commented Nov 11, 2021

I would appreciate it very much if @japgolly can use their knowledge of the scalajs-react library to add some clarity to this discussion.

This is a genuine question. I would like to know exactly how the definition

def onClick(e: org.scalajs.dom.Event): IO[Unit]

is supposed to function as an onClick listener, when it only produces a value (description of an action) and doesn't actually do anything (the actual action), which kind of clashes with the JS execution model where we have to immediately do something based on events, otherwise they are propagated further/expired by the time the IO runs.

Thanks in advance.

@vasilmkd
Copy link
Member

vasilmkd commented Nov 11, 2021

The only way I can see this happening if the React interop looks like this (excuse the hand wavy syntax):

function somewhereInReact(event) {
  const io = SomewhereInScala.onClick(event)
  io.unsafeRunAndForget()
}

And the only reason why e.preventDefault() would have ever worked in this type of interop is if unsafeRunAndForget() ran the IO immediately until the first asynchronous boundary, which would have executed the IO.delay(event.preventDefault()) before doing the rest of the IO (which admittedly is how CE2 is executed).

So, in a sense, the library relies that this method of execution is used. This isn't a problem per se, we just maybe have to include IO#unsafeRunHere as an option, and then scalajs-react can do the correct configuration that some effects (like event handlers) are executed in that fashion (or maybe even all effects, but I dare not speculate that much at this point).

Edit:
Of course, I may have misunderstood something and might be completely off base here.

@japgolly
Copy link

Hey @vasilmkd , sorry I missed your question here.

We don't have def onClick(e: org.scalajs.dom.Event): IO[Unit] in scalajs-react. Have a look at how onClick is used in this example.

One could write <.button(^.onClick --> IO(println("hello"))) in scalajs-react which is translated to something effectively like

const run = ...
const io = IO(println("hello"))
<button onClick={() => run(io)} />

So to work backwards:

@japgolly
Copy link

scalajs-react also directly accepts SyncIO instances as event handlers. But that being said, the more synchronicity we can get from IO execution, the better.

@vasilmkd
Copy link
Member

@japgolly Thanks for going into detail. Can you maybe confirm then that the default implementation of preventDefault in scalajs-react using Cats Effect 3 IO doesn't work properly because of the asynchronous execution?

@japgolly
Copy link

It's up to users wrap e.preventDefault() (for some event e) in whichever effect type they like. So users can do Callback(e.preventDefault()), SyncIO(e.preventDefault()), IO(e.preventDefault()), and then it's translated to JS using the def dispatch method on the implicit effect instances for the effect type. If a user types IO(e.preventDefault()) it will be run using unsafeRunAndForget.

@japgolly
Copy link

Oh and btw, I'd be very happy to change scalajs-react if there's a better way to execute these effects. It would be a binary-compatible change so np, just let me know

@vasilmkd
Copy link
Member

@japgolly Can users write an onClick handler in SyncIO while the other 99% of their application uses IO without major hassles?

@armanbilge
Copy link
Member

I think if you want your onClick handler to do something async you'll need IO.

@djspiewak
Copy link
Member

Oh and btw, I'd be very happy to change scalajs-react if there's a better way to execute these effects. It would be a binary-compatible change so np, just let me know

Actually if you change action.unsafeRunAndForget() to the following, it should resolve the issue for most users:

action.syncStep.unsafeRunSync().fold(_.unsafeRunAndForget(), _ => ())

What we're kind of getting at is whether or not this should be made more convenient.

@vasilmkd
Copy link
Member

I think if you want your onClick handler to do something async you'll need IO.

Right, I very much support this, but IMO the logic cuts both ways. If people want to execute something synchronously, they should really be reaching for SyncIO. I think that it's not a big leap to assume that those same users wouldn't reach for scala.concurrent.Future in that scenario. IMO, a part of this problem can be solved with better outreach and teaching.

@armanbilge
Copy link
Member

@djspiewak except of course the danger of that is that is that we no longer have auto-yielding. Which might not be important for most uses as you say.

@armanbilge
Copy link
Member

armanbilge commented Nov 29, 2021

Right, I very much support this, but IMO the logic cuts both ways. If people want to execute something synchronously, they should really be reaching for SyncIO

Yes, except how do you combine both? Say you want to do e.preventDefault() synchronously and then start doing some async stuff.

@japgolly
Copy link

Yes, except how do you combine both? Say you want to do e.preventDefault() synchronously and then start doing some async stuff.

I typically use Callback and AsyncCallback which are analogous to SyncIO and IO, but in practice it's very, very common that I mix sync and async effects in event handlers in my real-world webapps.

@vasilmkd
Copy link
Member

Hot take:

A mix of synchronous and asynchronous execution is what I'm naming "synchronously starting the execution of an asynchronous effect", which is exactly Daniel's code

action.syncStep.unsafeRunSync().fold(_.unsafeRunAndForget(), _ => ())

and its type is SyncIO[Unit].

@vasilmkd
Copy link
Member

Anything else is relying on implementation details.

@djspiewak
Copy link
Member

What if we added a step limiter overload to syncStep? So basically allowing users to request a maximum number of synchronous steps before inserting an artificial yield.

@armanbilge
Copy link
Member

armanbilge commented Nov 29, 2021

and its type is SyncIO[Unit]

I think it needs some rearranging to get that type, but it can only become SyncIO if you hide the start of an async effect inside of a delay. Which feels kind of wrong?

What if we added a step limiter overload to syncStep? So basically allowing users to request a maximum number of synchronous steps before inserting an artificial yield.

Yes, I like this idea 👍

@japgolly
Copy link

Yeah, the think the detail and new approach is fine.

Also, as an end-user without very detailed knowledge of CE, I personally think it'd be pretty cool to have a method that wraps action.syncStep.unsafeRunSync().fold(_.unsafeRunAndForget(), _ => ()) so vote 1 from me for a convenience method 😊

@armanbilge
Copy link
Member

armanbilge commented Nov 29, 2021

And we come full circle 😉 that's what I proposed in #2478.

@vasilmkd
Copy link
Member

I'm not against a convenience method for this. I much prefer it over changing the execution model of IO.

@armanbilge
Copy link
Member

I'd like to also use Daniels step-limiter idea, and ideally the convenience method can just grab it from it the IORuntime.

@djspiewak
Copy link
Member

The convenience method is just a little scary because it's so profoundly different from the normal execution approach. Not saying it's a wrong thing to do, it's just really really different and it feels better to make it explicit so users can't get into this mode by accident.

@armanbilge
Copy link
Member

I think it is much more dangerous on JVM than JS, and that's why I proposed we make the convenience method JS only.

@djspiewak
Copy link
Member

unsafeRunWithSyncPrefix? I guess I'm semi convinced. Bike shed time.

@djspiewak
Copy link
Member

What about unsafeRunSyncToFuture(): Future[A]? This gives you the same functionality as unsafeToFuture, except that it runs the prefix synchronously.

@armanbilge
Copy link
Member

I do like that one :) in any case, it will need docs!

@matil019
Copy link
Author

Not every IOs can be created manually. For example, fs2.Stream seems to put actions behind an async boundary no matter what.

DataTransfer.items must be used synchronously just like e.preventDefault(). I forgot the exact piece of code but something like Stream.range(0, dataTransfer.items).evalMap(i => IO(dataTransfer.items(i))) ... .compile.drain didn't work. So I gave up using syncStep and use something like SyncIO[Stream[IO, A]].

Sorry I should have mentioned this earlier but I was reluctant to create a complete example, but I didn't want to miss the discussion here.

@armanbilge
Copy link
Member

@matil019 how long is dataTransfer.items? Besides the initial yield, IO also implements auto-yielding which will interrupt an otherwise synchronous chain of IOs.

@matil019
Copy link
Author

matil019 commented Nov 30, 2021

@armanbilge its length is the number of items dragged and dropped so it's variable. I experienced the problem even if the length is only 1.

If this relevant: I used to add bufferAll to collect all of the elements into buffer before encountering an async boundary. Here is a part of a concrete code, which worked fine in CE2 but not in CE3:

Stream.range(0, dataTransfer.items.length)
  .map(i => dataTransfer.items(i))
  .evalMap(item => IO(item.getAsFileSystemHandle().toFuture))
  .bufferAll
  .evalMap(fut => IO.fromFuture(IO(fut)))
  // map, flatMap, etc.

That said, if auto-yielding is a thing, I don't see the point of syncStep. Executing a certain part of actions synchronously is an absolute requirement for me. SyncIO is reliable in that regard but IO#syncStep is not.

EDIT: Does IO#syncStep ignore auto-yielding and just look at the data structure of IO values? If so, I'm not really sure why the above code of mine didn't work with syncStep.

@armanbilge
Copy link
Member

Thanks for the details! Not immediately clear to me why it's not working for you, but I'm not an expert on how those fs2 apis are implemented and it's possible something changed.

Does IO#syncStep ignore auto-yielding and just look at the data structure of IO values?

Yes, syncStep does not do any auto-yielding. So I'm also not sure why it didn't work. Can you show how you were using syncStep?

Executing a certain part of actions synchronously is an absolute requirement for me.

Yep, we've created #2610 with a proposal to try and solve that problem as well.

@djspiewak
Copy link
Member

Doesn't Stream.eval introduce an async barrier by raceing a cancelation effect against the main one? That might be old. Pinging @diesalbla for thoughts.

@diesalbla
Copy link

diesalbla commented Nov 30, 2021

Doesn't Stream.eval introduce an async barrier by raceing a cancelation effect against the main one? That might be old. Pinging @diesalbla for thoughts.

So, here is the code that "compiles" (in fs2 jargon) a Stream.eval, which is represented as an object of the Eval subclass of Pull, into the target effect type F: https://github.com/typelevel/fs2/blob/main/core/shared/src/main/scala/fs2/Pull.scala#L1027-L1036. It makes a call to the Scope.interruptibleEval method, which then refers to the InterruptibleContext.eval function. It is in the code of that last eval that the race you are referring to occurs.

    * When the stream is evaluated, there may be `Eval` that needs to be cancelled early,
    * when scope allows interruption.
    * Instead of just allowing eval to complete, this will race between eval and interruption promise.
    * Then, if eval completes without interrupting, this will return on `Right`.

@armanbilge
Copy link
Member

armanbilge commented Nov 30, 2021

Thanks! @matil019 it seems Stream is not quite suitable in this case, although I wonder if you can just use Traverse :)

@matil019
Copy link
Author

matil019 commented Dec 1, 2021

@armanbilge Thanks for suggesting a workaround. Giving up the convenience of fs2.Stream to run certain actions synchronously is a huge tradeoff, though. Also, if you mean returning IO[List[A]] instead of SyncIO[Stream[IO, A]], I'm worried about the fact that the IO must be run with syncStep becomes invisible from its type.

P.S. would you still like me to post the real usage of syncStep?

@armanbilge
Copy link
Member

@matil019 it's a bit tricky, but if you want to use Stream you can use the Sync compiler like this:

.compile(Compiler.target(Compiler.Target.forSync))

That way it will not use any Concurrent operations, and using syncStep will hopefully do want you want. Give it a try?

Note that using the Sync compiler has its own complications (e.g. typelevel/fs2#2371), so this is a bit hacky ... unfortunately it seems tradeoffs are the game here.

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

Successfully merging a pull request may close this issue.

6 participants