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

Option to failFast on zip and sequence #8994

Closed
scabug opened this issue Nov 18, 2014 · 13 comments
Closed

Option to failFast on zip and sequence #8994

scabug opened this issue Nov 18, 2014 · 13 comments

Comments

@scabug
Copy link

scabug commented Nov 18, 2014

It should be possible to supply a flag to fail fast on both zip and sequence operations.

val a = Future{Thread.sleep(5000);1}
val b = Future{Thread.sleep(1000); throw new Exception("test")}
val c = a zip b

c will fail in 5 seconds with the exception from B.

Most clients would probably prefer it to fail in 1 second even if that means that they might get an exception from Future b instead of a possible exception from Future a.

@scabug
Copy link
Author

scabug commented Nov 18, 2014

Imported From: https://issues.scala-lang.org/browse/SI-8994?orig=1
Reporter: @jedesah
Affected Versions: 2.11.4

@scabug
Copy link
Author

scabug commented Nov 18, 2014

@jedesah said:
I would be glad to issue the PR if this is deemed useful.

@scabug
Copy link
Author

scabug commented Nov 25, 2014

@retronym said (edited on Nov 25, 2014 5:16:30 AM UTC):
@phaller WDYT?

@scabug
Copy link
Author

scabug commented Aug 7, 2015

Oscar Boykin (oscar) said:
Implemented such a method here:

https://github.com/twitter/scalding/pull/1412/files

I'll try to make a PR to the scala repo to discuss mainlining this.

@julienrf
Copy link

julienrf commented Oct 20, 2020

I think this is an important issue. In addition to this, I’ve noticed that zip is not commutative “fail-wise”.

See this snippet:

import scala.concurrent.ExecutionContext.Implicits.global
import scala.concurrent.{ Await, Future, blocking }
import scala.concurrent.duration.Duration

def failing() = Future.failed(new Exception("KO"))
def succeeding() = Future(blocking(Thread.sleep(5000)))

System.currentTimeMillis() / 1000 // : Long = 1603205724
Await.ready(failing().zip(succeeding()), Duration.Inf)
System.currentTimeMillis() / 1000 // : Long = 1603205724
Await.ready(succeeding().zip(failing()), Duration.Inf)
System.currentTimeMillis() / 1000 // : Long = 1603205729

Note that executing failing().zip(succeeding()) completes instantaneously, whereas executing succeeding().zip(failing()) completes only after succeeding() completes.

I think we should fix zip such that a.zip(b) fails as soon as a or b fails. We should also make sure that sequence and traverse are fixed.

@julienrf
Copy link

@johnynek do you want to contribute your fix?

@SethTisue
Copy link
Member

perhaps @viktorklang has a take on this

@viktorklang
Copy link

@SethTisue @julienrf I definitely don't mind fail-fast behavior added—assuming that it can be done in a resource-efficient way which does not lead to performance degradations for existing code.

@johnynek
Copy link

In the code linked above, we use futures to wait on hadoop jobs finishing. In such cases, the real cost is running a job you know you don't need. This is not the usual case with futures in scala.

My solution is fairly straight forward. I think you can improve the performance but my assumption is the linked code above will reduce performance of zip.

@julienrf
Copy link

In the code linked above, we use futures to wait on hadoop jobs finishing. In such cases, the real cost is running a job you know you don't need. This is not the usual case with futures in scala.

I think this is out of the scope of Future (although I would love to see this concern addressed in the scala-library!)

My solution is fairly straight forward. I think you can improve the performance but my assumption is the linked code above will reduce performance of zip.

OK, then we need to run some benchmarks to see if the performance reduction would be significant or not.

@viktorklang
Copy link

Note that we're talking about changing the implementation of zipWith—since zip uses zipWith. Also, I do not see a way to implement it generically on Future since it will require a concrete implementation of Promise, which trait Future cannot assume.

@eloots
Copy link

eloots commented Oct 20, 2020

Give the users of the library a choice; those who want 'the performance' can choose the current implementation. For other use cases that require a fail fast behaviour, use a new implementation (zipWithFailFast, Future.sequenceFailFast, ...).
There are valid use cases for both.

I noticed Future.sequence not failing fast a long time ago. See here for an implementation of Future.sequenceFailFast (Scala 3 code).

@julienrf
Copy link

julienrf commented Jun 7, 2021

Closed by scala/scala#9655

@julienrf julienrf closed this as completed Jun 7, 2021
@SethTisue SethTisue modified the milestones: Backlog, 2.13.7 Jun 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants