Commits on Jun 7, 2021

1. Fix asymmetric failure behavior of Future#{zip,zipWith,traverse,seque… …

   …nce} by making them fail fast regardless of ordering
   
   Currently, given the following setup:
   
   ```scala
   val f1 = Future{Thread.sleep(10000)}
   val f2 = Future{Thread.sleep(2000); throw new Exception("Boom")}
   ```
   
   The following two snippets exhibit different failure behavior:
   
   ```scala
   val fa = Await.result(f1.zip(f2). Duration.Inf)
   ```
   
   ```scala
   val fb = Await.result(f2.zip(f1). Duration.Inf)
   ```
   
   `fa` fails after 10000ms, while `fb` fails after 2000ms. Both fail with `java.lang.Exception: boom`.
   
   When zipping two `Future`s together, if the left `Future` fails early, the zipped `Future` fails early. But if the right `Future` fails early, the zipped `Future` waits until the right `Future` completes before failing.
   
   `traverse` and `sequence` are similarly implemented with `zipWith` and should exhibit the same behavior. This all arises because `zipWith` is implemented using `flatMap`, which by definition asymmetric due to waiting fo the left `Future` to complete before even considering the right `Future`.
   
   The current behavior makes the failure behavior of `Future`s most unpredictable; in general nobody pays attention to the order of `Future`s when zipping them together, and thus whether a `zipWith`ed/`zip`ed/`traverse`d/`sequence`d `Future` fails early or not is entirely arbitrary.
   
   This PR replaces the implementation of `zipWith`, turning it from `flatMap`-based to `Promise`-based, so that when a `Future` fails early, regardless of whether it's the left or right `Future`, the resultant `Future` will fail immediately.
   
   Implementation-wise I'm using an `AtomicReference` and `compareAndSet`, which should give us the behavior we want without any locking. It may well be possible to achieve with even less overhead, e.g. using only `volatile`s or even using no concurrency controls at all, but I couldn't come up with anything better. If anyone has a better solution I'll happily include it.
   
   This fix would apply to all of `zip`/`zipWith`/`traverse`/`sequence`, since they all are implemented on top of `zipWith`
   
   While it is possible that someone could be relying on the left-biased nature of current `zip`/`zipWith`/`traverse`/`sequence` implementation, but it seems like something that's unlikely to be reliable enough to depend upon. In my experience people generally aren't aware that `zipWith`/`zip`/`traverse`/`sequence`, and they don't generally know the total ordering of how long their Futures take to run. That means status quo behavior would just result in some `Future` fails mysterious taking longer to report for no clear reason.
   
   Notably, the biased nature of these operators is not documented in any of their scaladoc comments.
   
   While there is a non-zero chance that somebody could be intentionally or unintentionally depending on the biased nature of these combinators, there is a much greater chance that someone unaware of the current bias would be puzzled why their highly-concurrent system seems to be taking longer than expected in certain scenarios. It seems likely that this PR would fix more bugs than it would introduce
   
   Note that this does not fix the left-biased fail-fast behavior of `flatMap` chains, or their equivalent `for`-comprehensions, as `flatMap`'s API is inherently left-biased. But anyone who wants fail-fast behavior can convert sections of their `flatMap` chains into `.zip`s where possible, and where not possible that's generally because there is some true data dependency between the `flatMap`s

   

   lihaoyi committed Jun 7, 2021
   Configuration menu

   • View commit details
   • Copy full SHA for 0fc323b
   • Browse repository at this point

   Copy the full SHA

   0fc323b View commit details

   Browse the repository at this point in the history