Incorrect Async scaladoc

[durban]

I think the Async scaladoc contains incorrect information since #3002 was merged. I've already commented on that PR about this issue, but the problem is basically that (contrary to its scaladoc) async_ does not return an uncancelable effect (and I don't think it's intended to, i.e., the scaladoc is mistaken):

scala> val tsk = for {
     |   fib <- (IO.asyncForIO.async_[Unit] { cb => () }).start
     |   _ <- IO.sleep(2.seconds)
     |   _ <- fib.cancel
     |   _ <- fib.join
     | } yield ()

val tsk: cats.effect.IO[Unit] = IO(...)

scala> tsk.unsafeRunSync()(cats.effect.unsafe.IORuntime.global)

// returns after 2 seconds

And similary for async when it returns None. (The note about async being uncancelable during its registration is correct, as far as I can tell.)

[armanbilge]

I've been confused about this before. For some reason I was under the impression that async_ returns an uncancelable effect, but I recently realized it returns a cancelable effect with no finalizer.

Edit: fwiw, at least to me it really seemed like it should be uncancelable by default unless explicitly made cancelable.

[TimWSpence]

Edit: fwiw, at least to me it really seemed like it should be uncancelable by default unless explicitly made cancelable.

Oh really? I thought basically everything was "cancelable by default"? The only exception I can think of is Resource acquisition, where it's just too likely to lead to bugs if people aren't very careful

[SystemFw]

There's a subtler point at play here as well. You can think of async as being "callback registration" + "waiting for callback to fire", async_ and async both offer cancelation* of the "waiting", with the latter also giving you the ability to attach a finaliser.
*The issue here is one of cancelation vs interruption, for lack of better terminology. Both let you "disconnect", but you cannot also actually interrupt the running effect unless you use async, i.e. that finaliser is often the thing that interrupts.
Btw as you can see the design space here is bigger than it seems, that's one of the reasons for Cont being the building block rather than async itself.

Neither lets you cancel the registration, that's what the new asyncPoll does. In other words @durban is correct on both points:

async_ does not return an uncancelable effect
The note about async being uncancelable during its registration is correct, as far as I can tell.

fwiw, at least to me it really seemed like it should be uncancelable by default unless explicitly made cancelable.

If you're referring to the "waiting", that's not the CE default, and also pretty weird for something async to be uncancelable. If you're referring to the "registration", what you describe is already what happens

[armanbilge]

The issue here is one of cancelation vs interruption, for lack of better terminology.

Yes, this. It feels very strange to me that you can cancel an async effect and move on without actually interrupting it. This seems very wrong, and as far as I know is the only place where these concepts are disconnected: everywhere else, cancellation means the fiber is interrupted and there is backpressure until it has done so.

[SystemFw]

Yes, this. It feels very strange to me that you can cancel an async effect and move on without actually interrupting it. This seems very wrong, and as far as I know is the only place where these concepts are disconnected: everywhere else, cancellation means the fiber is interrupted and there is backpressure until it has done so.

Honestly, I see where you're coming from (in my previous message I hadn't realised you meant async_ specifically). The semantics of async_ are not that weird in the Java world, but definitely inconsistent with the way cats-effect works in general. The question is which one is more confusing, and I don't have a great answer to that.

[armanbilge]

The question is which one is more confusing, and I don't have a great answer to that.

It's a question, but I'm not sure if it is the question 😛 I think my main observations are:

1. the backpressure-on-cancelation semantic is already confusing e.g.

   • Let's discuss the cancelation model #1979
   • Cancellation model still feels wrong #3190

2. if I've got an IO[Whatever] that does some I/O, then I expect the model to abstract me from whether it's implemented with IO.async_/IO.async or IO.blocking/IO.interruptible. But IO.async_ is not like the others with regard to cancelation, so the abstraction is leaky.

3. I don't see why the usual arguments for backpressure-on-cancelation should not apply to IO.async_.

All in all, it's really hard not to feel like it's a bug that IO.async_ is cancelable.

[djspiewak]

All in all, it's really hard not to feel like it's a bug that IO.async_ is cancelable.

This is a really interesting point. When I originally wrote async_, I was thinking of it more like flatMap, which is just… cancelable. But it's not really the same, since there is always some other thing that is holding the callback which is now pointing to a canceled fiber that hasn't been GC'd.

You can extend this argument further though, since even when you provide a finalizer using async, that finalizer often doesn't block awaiting confirmation that cancelation was accepted. A lot of async cancelation mechanisms are non-backpressured, and most people don't make any direct effort to create backpressure in their async finalizers, which in turn corrupts the model just a little bit.

[armanbilge]

that finalizer often doesn't block awaiting confirmation that cancelation was accepted.

Sure, but IMHO that's only because of bullshit APIs in-between. My work on fs2-io_uring convinces me that if we start from the primitives we can do this Right™.

In any case, regardless of what most people may do, I'm staring at this.

cats-effect/kernel/shared/src/main/scala/cats/effect/kernel/Async.scala

Lines 158 to 161 in 5ba5543

	def fromFuture[A](fut: F[Future[A]]): F[A] =
	flatMap(fut) { f =>
	flatMap(executionContext) { implicit ec =>
	async_[A](cb => f.onComplete(t => cb(t.toEither)))

and at this:

cats-effect/kernel/js/src/main/scala/cats/effect/kernel/AsyncPlatform.scala

Lines 25 to 27 in 5ba5543

	def fromThenable[A](iot: F[Thenable[A]]): F[A] =
	flatMap(iot) { t =>
	async_[A] { cb =>

Even if we are not going to fix async_ (and I would sleep much better if we did, or we deprecated it in favor of a new ugly-named method that is uncancelable) then at least we can fix how we use it ourselves.

[SystemFw]

I think historically, async_ comes from older cats-effect and Monix, i.e. Alex's previously held opinion that backpressuring on finalisers is not a good idea (or at least not a good default)

[SystemFw]

Rereading the whole thread though, I think I actually agree with Arman

[djspiewak]

Does anybody have any good intuition about what would happen if we just… flipped the switch? Specifically, making async_ uncancelable.

[armanbilge]

If you think the Queue changes needed an RC, this definitely would need one. I'd be surprised if I haven't depended on this behavior in various places. For example fixing typelevel/fs2#2968 broke http4s which was actually another FS2 bug in typelevel/fs2#2972,

[djspiewak]

Oh this would definitely need a dedicated RC cycle and a lot of communication. It's out for 3.4. Also just playing around with it quickly on a branch, it breaks Cats Effect itself. Probably just a bunch of bad assumptions in a few places, but it's illustrative that even in CE itself we are leaning on this semantic.

[armanbilge]

Now that #3205 is merged I believe the scaladoc is correct, at least on the series/3.x branch. 3.4.x is in a bit of a weird place since the scaladoc expresses the correct intent, but the implementation is bugged.