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

`flatMap` cancels all involved futures #390

Merged
merged 3 commits into from Aug 1, 2017

Conversation

Projects
None yet
3 participants
@larsrh
Contributor

larsrh commented Jul 31, 2017

Fixes #381

@larsrh

This comment has been minimized.

Contributor

larsrh commented Jul 31, 2017

Error in Travis is because of binary incompatibility. I wasn't aware that returning a more specific type could cause this.

@codecov

This comment has been minimized.

codecov bot commented Jul 31, 2017

Codecov Report

Merging #390 into master will increase coverage by 0.16%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #390      +/-   ##
==========================================
+ Coverage   87.49%   87.66%   +0.16%     
==========================================
  Files         293      293              
  Lines        8114     8112       -2     
  Branches     1156     1149       -7     
==========================================
+ Hits         7099     7111      +12     
+ Misses       1015     1001      -14
@@ -115,18 +116,28 @@ object FutureUtils {
*
* Similar to `Future.transformWith` from Scala 2.12.
*/
def transformWith[A,B](source: Future[A], f: Try[A] => Future[B])(implicit ec: ExecutionContext): Future[B] = {
def transformWith[A,B](source: Future[A], f: Try[A] => Future[B])(implicit ec: ExecutionContext): CancelableFuture[B] = {

This comment has been minimized.

@ktoso

ktoso Aug 1, 2017

Chiming in since asked on twitter https://twitter.com/ktosopl/status/892221038242570240

This is binary incompatible, MiMa is right.
Changing the type like this will cause NoSuchMethodError.

Simple way to test those things if you think MiMa may be mistaken:

$ ls *scala
it.scala    lol.scala   main.scala
ktoso @ 三日月/tmp
$ cat *scala
object f { def f(): A = null }

trait A
trait B extends A

object Main extends App {

 val a =   f.f()
}
ktoso @ 三日月/tmp
$ scalac *.scala
ktoso @ 三日月/tmp
$ java -cp /Users/ktoso/.ivy2/cache/org.scala-lang/scala-library/jars/scala-library-2.12.2.jar:. Main
ktoso @ 三日月/tmp
$ vim it.scala
ktoso @ 三日月/tmp
$ scalac it.scala
ktoso @ 三日月/tmp
$ cat it.scala
object f { def f(): B = null }

ktoso @ 三日月/tmp
$ java -cp /Users/ktoso/.ivy2/cache/org.scala-lang/scala-library/jars/scala-library-2.12.2.jar:. Main
Exception in thread "main" java.lang.NoSuchMethodError: f$.f()LA;

Hope this helps

This comment has been minimized.

@larsrh

larsrh Aug 1, 2017

Contributor

Yeah, it helps. I'll have to rework this change to make it binary compatible.

@alexandru WDYT?

This comment has been minimized.

@alexandru

alexandru Aug 1, 2017

Member

Thanks @ktoso for your input. @larsrh I've written a comment below.

This comment has been minimized.

@ktoso

ktoso Aug 1, 2017

Sounds like good timing if you can break now 👍

@alexandru

This comment has been minimized.

Member

alexandru commented Aug 1, 2017

@larsrh this change is for master and master has upgraded to depending on cats-core directly.

This means that binary compatibility is already broken heavily, because monix-cats, monix-scalaz and monix-types sub-projects are now gone, along with the instances defined for Task, Coeval and Observable. And because we are doing this, this is also a good time to get rid of deprecated methods, which lingered in the codebase for preserving compatibility.

This is why we have a big list of incompatibilities documented for 3.0.0.

So don't worry about it. If there's a time to break binary compatibility, it is now.
But Mima still serves a purpose here, because all breaking changes need to be documented.

p.future
source match {
case c: Cancelable => cancelable.orderedUpdate(c, order = 1)

This comment has been minimized.

@alexandru

alexandru Aug 1, 2017

Member

This is correct, but you already have source when initializing your val cancelable so it might be a better idea to initialize that as MultiAssignmentCancelable(source) (thus source being an initial underlying reference), because we avoid doing an Atomic.compareAndSet that way.

And so instead of the second orderedUpdate(order = 2), you can just use :=, because orderedUpdate also implies some extra logic.

This comment has been minimized.

@larsrh

larsrh Aug 1, 2017

Contributor

Will update accordingly.

@@ -161,6 +172,10 @@ object FutureUtils {
p.future
}
/** Creates a future that never completes. */
def never[A]: Future[A] =
Promise[A].future

This comment has been minimized.

@alexandru

This comment has been minimized.

@ktoso

ktoso Aug 1, 2017

Since I got notified since I chimed in about the MiMa thing thought I'll share some insight here too. Feel free to tell me to go away if you'd prefer that :-)

I would say 👎, since this can create memory leaks in the form of callbacks registered on this future, which will never trigger, however still will be registered and kept in memory.

Explanations: http://viktorklang.com/blog/Futures-in-Scala-2.12-part-6.html
Scala 2.12 solution: https://github.com/scala/scala/blob/2.12.x/src/library/scala/concurrent/Future.scala#L561

Sneaky little edge case, isn't it

This comment has been minimized.

@larsrh

larsrh Aug 1, 2017

Contributor

I only use this for testing. I could move this to the test suite, such that nobody will accidentally use it.

This comment has been minimized.

@larsrh

larsrh Aug 1, 2017

Contributor

The other option would be copy-pasting this from Scala into Monix, but I'm not sure whether that's a good idea.

This comment has been minimized.

@ktoso

ktoso Aug 1, 2017

Yeah if it's just in tests then ok I guess, would move it out from src/main though then. Your call :)

This comment has been minimized.

@alexandru

alexandru Aug 1, 2017

Member

OH, I now see that it is basically never from Scala 2.12. Nice.

This comment has been minimized.

@larsrh

larsrh Aug 1, 2017

Contributor

@alexandru I'll use that, then.

This comment has been minimized.

@alexandru

alexandru Aug 1, 2017

Member

But then never from Scala 2.12 overrides everything else, e.g. flatMap.

@ktoso If we aren't overriding the other methods likeflatMap, I think that represents a potential memory leak as well.

@larsrh we might need to do a Scala 2.11 specific object (in a scala_2.11 directory) and just rely on Scala's never for 2.12.

This comment has been minimized.

@alexandru

alexandru Aug 1, 2017

Member

@ktoso sorry for the noise, just wanted to mention that your input is greatly appreciated. Thanks.

This comment has been minimized.

@ktoso

ktoso Aug 1, 2017

No problem, cheers!

@larsrh

This comment has been minimized.

Contributor

larsrh commented Aug 1, 2017

I've addressed all feedback & Travis is green.

@alexandru alexandru merged commit 27c1508 into monix:master Aug 1, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@larsrh larsrh deleted the larsrh:topic/cancel-flatmap branch Aug 1, 2017

@larsrh

This comment has been minimized.

Contributor

larsrh commented Aug 1, 2017

@alexandru Are you planning to release milestone builds of 3.0.0?

@alexandru

This comment has been minimized.

Member

alexandru commented Aug 1, 2017

@larsrh yes, snapshots get published automatically whenever I merge master into snapshot.

I started a new build, when finished you should have a new version published if you want to play: https://travis-ci.org/monix/monix/builds/259733983

@larsrh

This comment has been minimized.

Contributor

larsrh commented Aug 1, 2017

Your publish script might be failing:

java.lang.IllegalStateException: Ivy file not found in cache for io.monix#monix-eval_2.11;3.0.0-27c1508!

(see build log)

@alexandru

This comment has been minimized.

Member

alexandru commented Aug 1, 2017

@larsrh unfortunately I've been having issues due to tooling, like Coursier, not recognizing empty projects (i.e. projects where the POM itself is the artifact,with no JAR file) and the root monix project is one.

Cannot reproduce it locally, but I've just pushed an attempted fix for it, a build is in progress: https://travis-ci.org/monix/monix/builds/259961033

@alexandru

This comment has been minimized.

Member

alexandru commented Aug 1, 2017

@larsrh the latest build succeeded after those fixes, you should see 3.0.0-22bf9c6 on Maven Central.

@larsrh

This comment has been minimized.

Contributor

larsrh commented Aug 2, 2017

Indeed, thanks!

@alexandru alexandru added this to the 3.0.0 milestone Jan 21, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment