Cats-effect: Implement Async#promise as a lower level primitive

[djspiewak]

trait Async[F[_]] extends Temporal[F] with Sync[F] {
  // ...
  def promise[A]: F[Promise[F, A]]

  def async[A](k: (Either[Throwable, A] => Unit) => F[Option[F[Unit]]]): F[A] =
    promise[A] flatMap { p => 
      k(_.fold(p.fail, p.complete)) flatMap {
        case Some(fin) => p.get.onCancel(fin)
        case None => p.get
      }
    }
}

trait Promise[F[_], A] {
  def fail(t: Throwable): Unit
  def complete(a: A): Unit
  def get: F[A]
}

This is a lower level primitive, similar to Deferred (but not identical!), which allows us to solve the problem of composable Async instances (e.g. #1086). Additionally, this gives us a lot more control over cancelation semantics and radically simplifies concrete implementations, which proves that it's the more primitive version of the concept.

[RaasAhsan]

To reduce burden on existing implementations, I wonder if it's possible to define promise in terms of async somehow? So you could choose to implement either, sort of like the situation between map/join and flatMap.

[djspiewak]

By capturing a hidden var in the Promise instance, you can implement promise in terms of async. This is definitely something implementations can do; I'm not sure whether or not we should do it for them. It's a good question though.

[djspiewak]

Side-note fix for async which avoids issues leaking the cancel token:

  def async[A](k: (Either[Throwable, A] => Unit) => F[Option[F[Unit]]]): F[A] =
    uncancelable { poll =>
      promise[A] flatMap { p => 
        k(_.fold(p.fail, p.complete)) flatMap {
          case Some(fin) => poll(p.get).onCancel(fin)
          case None => poll(p.get)
        }
      }
    }

This also makes the registration function uncancelable, which is a semantic change, but arguably a necessary one.

[SystemFw]

This also makes the registration function uncancelable, which is a semantic change, but arguably a necessary one

you can still recover the old behaviour if needed right? just not by calling async directly. I don't think it's going to be a problem anyway but I need to think it through more given that ce3 async returns an F.

[RaasAhsan]

I think we can optimize async_ to that end

[djspiewak]

Yep! One of the nice things about this is it splits async_ from async and makes the former much more efficient.

[RaasAhsan]

One thought I have is that if the caller doesn't supply a finalizer to async (so they pass in None), uncancelable isn't really necessary because no finalizer will be registered. What if we split that out into one more function?

def async[A](k: (Either[Throwable, A] => Unit) => F[F[Unit]]): F[A]
def async0[A](k: (Either[Throwable, A] => Unit) => F[Unit]): F[A]
// defined in terms of `async0`
def async_[A](k: (Either[Throwable, A] => Unit) => Unit): F[A]

Though, on the other hand, it means that the returned effect inasync0 can cancel. I wonder if the semantics should dictate that the entire effect is uncancelable, regardless of whether there is a finalizer or not?

[djspiewak]

I feel like having the async0 variant would be confusing. Also, if the user doesn't have warn-on-discard enabled, they could end up silently losing their F[Unit] cancel token. I think that if someone really, really needs a cancelable registration semantic computed within an F context, they could use promise directly.

[djspiewak]

@SystemFw It may be worth addressing #1118 as part of this same task.

[SystemFw]

I'll look into that

[RaasAhsan]

IMO #1118 is an optimization and can probably be addressed independently after this PR

[djspiewak]

IMO #1118 is an optimization and can probably be addressed independently after this PR

Also a valid option, I was just thinking it might be relatively straightforward to do at the same time. We certainly don't want to do #1118 before this PR, since it would trivially conflict.

[SystemFw]

What should be the semantics for get? Should it be called at most once?

[djspiewak]

What should be the semantics for get? Should it be called at most once?

I think it's okay for it to be idempotent. That makes the state machine a bit simpler and matches the semantics of scala.concurrent.Promise

[SystemFw]

I think it's okay for it to be idempotent.

That sounds weird to me, so I'd like to understand the rationale a bit better. Feels like we are introducing a level of sharing in the runloop that shouldn't be there

That makes the state machine a bit simpler and matches the semantics of scala.concurrent.Promise

heh, as a consequence of the above sentence, I'm growing increasingly uncomfortable calling this construction Promise, feels like it's subtly different, like your previous name cont seemed to imply.

unless you are looking at it as a Fiber with no outcome, but then for example, what if I call get twice, and cancel one. Should the runloop be canceled? Should the other get be canceled? Feels like we need to then replicate Fiber semantics (or at least define the semantics for this operation)

[djspiewak]

That sounds weird to me, so I'd like to understand the rationale a bit better. Feels like we are introducing a level of sharing in the runloop that shouldn't be there

Are we? I mean, we already have the AsyncState machinery for caching result values (due to delays on registration completion). All this would mean is that we don't null out that cache when get is called, we just make sure the cache's GC path runs through the Promise instance itself.

I'm just basically thinking about the usability of the get abstraction. If we can keep the F[A] semantics as stateless as possible, it feels like it will be easier to use it correctly (for the same reason that MVar is tremendously difficult).

heh, as a consequence of the above sentence, I'm growing increasingly uncomfortable calling this construction Promise, feels like it's subtly different, like your previous name cont seemed to imply.

I'd like to explore this a bit. :-) How is it different? It seems almost identical to me.

unless you are looking at it as a Fiber with no outcome, but then for example, what if I call get twice, and cancel one. Should the runloop be canceled? Should the other get be canceled? Feels like we need to then replicate Fiber semantics (or at least define the semantics for this operation)

Ah, I think I see the confusion! So I would expect get.start.flatMap(_.cancel) to cancel the fetching of the value, not cancel the computation. Promise itself wouldn't have any notion of cancelation whatsoever, since it's just a mechanism for passing a value from a callback… back into the runloop. So in other words, you can cancel the get, but you can't use Promise to cancel the callback itself. In order to do anything like that, you would need to sequence an onCancel somewhere which handles the deregistration. This is why the runloop is greatly simplified by Promise, since async cancelation completely disappears as a concept.

[SystemFw]

So I would expect get.start.flatMap(_.cancel) to cancel the fetching of the value, not cancel the computation

ah I see, that makes sense. It just needs appropriate warnings that this construction is lower level and potentially leaky if you don't pay attention.

There is another minor point that irks me: when porting Deferred, we opted to not use the name Promise due to confusion with stdlib, and now we're "wasting" it for a low level construction that most users will never see, or worse try to use when they should reach for Deferred instead. I think I would prefer a name that conveys this angle:

since it's just a mechanism for passing a value from a callback… back into the runloop.

rather than a general promise type, but I can live with it

[djspiewak]

There is another minor point that irks me: when porting Deferred, we opted to not use the name Promise due to confusion with stdlib, and now we're "wasting" it for a low level construction that most users will never see, or worse try to use when they should reach for Deferred instead. I think I would prefer a name that conveys this angle:

That's a fair point. We could go with Cont, though I will point out that this is exactly Promise. :-) Which is precisely why it's somewhat unsafe. I'm not super-fond of the fact that Cont conflicts with cats. It's worth noting though that users probably will see this when they use Async, since it's somewhat more intuitive than async is.

[SystemFw]

since it's somewhat more intuitive than async is.

Sure, but I view that as a bug :P By which I mean that I'm fairly confident most users will get the definition of async based on promise wrong (like wrong poll, no onCancel etc), so it should feel scary, not intuitive, given that in most user facing code promise would be used to obtain async semantics.

I also have a lingering doubt about the sharing thing, but I cannot formulate it clearly yet, I think as I go further with the implementation is either going to become clearer, or prove to be my own misunderstanding

[djspiewak]

Sure, but I view that as a bug :P By which I mean that I'm fairly confident most users will get the definition of async based on promise wrong (like wrong poll, no onCancel etc), so it should feel scary, not intuitive, given that in most user facing code promise would be used to obtain async semantics.

Let's just call it "a Promise transformer" then. Everyone's scared of transformers. ;-)

[RaasAhsan]

I think the semantics of Promise do strongly agree with conventional definitions, but the name doesn't convey intent as well as it could. It's a special-purpose mechanism which we use to escape from the runloop to FFI-land and re-enter it later when we're ready to continue the effect.

If cont and Cont don't work, what about continue and Continue, or cont and Continue? I think those aren't reserved keywords

[SystemFw]

I like continue/Continue. resume/Resume could also be an alternative

[mpilquist]

:+1: on Continue or Resume and I agree that Promise is too alluring of a name to use for something so advanced. :)

[SystemFw]

Fixed by #1165