AsyncLaws.thrownInRegisterIsRaiseError cannot be upheld

[alexandru]

Hi guys,

We cannot have this law, described in this way:

  def thrownInRegisterIsRaiseError[A](t: Throwable) =
    F.async[A](_ => throw t) <-> F.raiseError(t)

I understand the intention here, but here's the problem: by contract (whether enforced by a law or not), the provided callback function must be called at most once, because that function might trigger side-effects, therefore, at least by convention, we need an idempotency guarantee to keep sanity.

Well, if you say that F.async[A](_ => throw t) must be equivalent to F.raiseError(t), then you cannot keep this idempotency guarantee without synchronization ... meaning for example to keep some state in an AtomicReference keeping track of calls (e.g. how Promise#trySuccess and Promise#tryFailure are working).

Example:

F.async { callback =>
  // Calling once
  callback(Right(1))
  // Oops, are we going to call our callback again?
  throw new RuntimeException("Bam, in your face!")
}

And while that would be cool, I'm pretty sure that you guys don't indent to introduce synchronization in order to achieve such idempotency guarantee, primarily because of performance reasons. In other words, it's better to keep this idempotency as a soft contract ... i.e. in our own implementation, we leave absolutely no room for violating it, but if the user does it, then the behaviour is undefined.

In Monix for such cases I prefer to simply do an ec.reportFailure(err), with that Task being non-terminating. In other words Task.async(_ => throw ex) is actually equivalent with Task.never.

I find that acceptable because ... (1) it doesn't blow the current call-stack, (2) it can be treated (with e.g. timeout and timeoutTo), (3) the exception gets reported by using the ExecutionContext so it has some visibility and (4) I find it normal in this case for the behaviour to be undefined.

Users should not allow exceptions to be thrown in register. If they do, they should do so at their own risk.

Think of this as a tradeoff: it's better to have the behaviour undefined (e.g. either never or raiseError, depending on what F[_] can do), than to risk calling that callback twice.

[djspiewak]

So, there is an idempotency check (specifically applied here). From a performance standpoint, I'm pretty sure AtomicBoolean is an order of magnitude faster than raw synchronization (especially when uncontested), but even beyond that, we could do even better with a method handle and a switch point if we really wanted to make it that fast.

The problem is that if IO doesn't do this, it either has to capture an ExecutionContext (in order to use reportFailure) or just swallow the error altogether, neither of which seem like particularly good solutions relative to the rest of the design.

But all that aside… I tend to agree that the law is too restrictive. It basically means that implementors can only implement their types in this way, and I don't think that's what we want to say. So I think you're right that we should remove the law and add it as a test specifically for IO.

[alexandru]

Well, yeah, on top of Java 8 that getAndSet operation is pretty fast, using platform intrinsics and if you do it once "per program" it's not a problem. Also when dealing with say a Scala Promise, it has trySuccess baked in.

So yes, it's not IO's implementation I'm complaining about, but this law :-)

It would have been helpful to specify an Either in that law ... such that we could specify equivalence with raiseError(ex) or never. Such a law would still disallow blowing the current call-stack for example.

[djspiewak]

It would have been helpful to specify an Either in that law ... such that we could specify equivalence with raiseError(ex) or never. Such a law would still disallow blowing the current call-stack for example.

Hmm, that's an interesting idea, though I'm not sure how many implementation flaws that would actually catch. A bigger problem though is that the current Eq[IO[A]] implementation I'm using in the laws is unable to check equivalence with never. I haven't actually looked at how Monix's tests do that…