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

Add conversions module for Java 8 CompletableFuture #584

Merged
merged 4 commits into from Feb 9, 2018

Conversation

Projects
None yet
3 participants
@oleg-py
Collaborator

oleg-py commented Feb 1, 2018

Closes #221

I could not build CancelableFuture -> CompletableFuture because cancelled CancelableFuture is non-terminating, which I cannot react on without something like Task.onCancelRaiseError. I went with Future -> CompletableFuture conversion instead.

There are also APIs I don't think we can support, like obtrudeValue which lets you mutate the result of CompletableFuture even when it is already completed. Luckily, even the doc for obtrudeValue says it guarantees nothing.

Also, a lot of CompletableFuture API allows supplying Executor to run on (and if you don't, it runs on synchronously or on a common ForkJoinPool) - and since Scheduler is an Executor too, I added deferCompletableFutureAction to Task companion.

Btw, although @alexandru said:

Given that Scala 2.12 requires Java 8, this should be added for Scala 2.12 only.

It should be possible to cross-build it for Scala 2.11.x too, since it is a separate module and there might be somebody running 2.11 on Java 8.

@codecov

This comment has been minimized.

codecov bot commented Feb 1, 2018

Codecov Report

Merging #584 into master will decrease coverage by 0.03%.
The diff coverage is 84.61%.

@@            Coverage Diff             @@
##           master     #584      +/-   ##
==========================================
- Coverage   90.69%   90.66%   -0.04%     
==========================================
  Files         370      373       +3     
  Lines        9846     9889      +43     
  Branches     1850     1847       -3     
==========================================
+ Hits         8930     8966      +36     
- Misses        916      923       +7
@alexandru

This comment has been minimized.

Member

alexandru commented Feb 1, 2018

We can describe onCancelRaiseError for CancelableFuture as well and we should probably add it:

import monix.execution.atomic.Atomic
import monix.execution.schedulers.TrampolineExecutionContext.immediate

def onCancelRaiseError[A](fa: CancelableFuture[A], e: Throwable)
  (implicit ec: ExecutionContext): CancelableFuture[A] = {

  if (fa.isComplete) fa else
    CancelableFuture.async { cb =>
      val active = Atomic(true)
      val cb2 = new RaiseCallback(active, cb)
      // Register listener
      fa.underlying.onComplete(cb2)(immediate)
      // Return cancelable
      new RaiseCancelable(active, cb)
    }
}

final class RaiseCallback[A](active: AtomicBoolean, cb: Try[A] => Unit)
  (implicit ec: ExecutionContext) extends (Try[A] => Unit) {
  
  def apply(value: Try[A]) =
    if (active.getAndSet(false)) {
      cb(value)
    } else value match {
      case Failure(e) => ec.reportFailure(e)
      case _ => ()
    }
}

final class RaiseCancelable[A](active: AtomicBoolean, cb: Try[A] => Unit)
  (implicit ec: ExecutionContext) 
  extends Cancelable {

  def cancel(): Unit = {
    if (active.getAndSet(false)) 
      ec.execute(new Runnable { 
        def run() = cb(Failure(new CancelledException()))
      })
  }
}

The nice thing about onCancelRaiseError is that this kind of synchronization is precisely what would have been needed if the contract would have been for CancelableError to always terminate in error on cancel, but this one is optional, on a by-need basis.

@alexandru

This comment has been minimized.

Member

alexandru commented Feb 1, 2018

So nice of you to pick this up 😀

Some ideas:

  1. I'd rather have this project named monix-java with the monix.java namespace, because the current Java is Java 9 and who knows, we might end up with Java 9 conversions when Java 10 is released and I'd rather not have multiple Java-related projects. We'll be conservative and mindful in what we accept too.

  2. I'd rather have its dependencies optional, as we might want to add conversions for monix.reactive as well - so each sub-project gets its own object / module under monix.java:

// For CancelableFuture
import monix.java.execution._

// For Task
import monix.java.eval._

And then in this sub-project's dependencies we can specify them as "provided". Not entirely sure if this is a good idea, since it can blow up in people's faces with a ClassNotFoundException, but we should include it in the monix package that brings in everything, anyway and "provided" is useful for people that want an a la carte experience.

@oleg-py

This comment has been minimized.

Collaborator

oleg-py commented Feb 1, 2018

onCancelRaiseError

That will only work if you call cancel on a result of CancelableFuture.onCancelRaiseError. But since Futures represent a computation that is already running, there's not much utility in this unless it propagates cancelled status downstream. Cancelled monix futures become non-terminating, so they propagate nothing. I cannot turn cancelled monix future into cancelled j.u.c.CompletableFuture.

So it's might be better to leave that as Future -> j.u.c.CompletableFuture that turns non-terminating Futures into non-terminating j.u.c.CompletableFutures - with no support for cancelling.

monix.java namespace

I'm slightly 👎 on that. First, if somebody does import monix._, that will shadow java package. Second, if we do add Java 9 conversions (e.g. to Flow interfaces, #438), it will be harder for users to understand why that ClassNotFoundException happens. I believe different namespaces (monix.java8, monix.java9) would be more convenient, even if it lives in the same module.

@oleg-py

This comment has been minimized.

Collaborator

oleg-py commented Feb 1, 2018

Hm, I cannot find a way to conditionally aggregate the subproject if scalaVersion used in build is not included in its crossScalaVersions. Is there a way?

@alexandru

This comment has been minimized.

Member

alexandru commented Feb 2, 2018

@oleg-py unfortunately you can't pick and choose the Scala versions per sub-project.

When I first wrote that description on the issue, I was thinking of a module inside monix-eval to only be included with Scala 2.12, not in a separate sub-project, but by using a source trick. Like we have for example here: https://github.com/monix/monix/tree/master/monix-execution/shared/src/main/scala_2.12/

This would be another way of approaching the problem. We could make object CancelableFuture extend a trait that's empty for Scala 2.11, but that contains extra utilities for Scala 2.12+.

On the monix.java namespace I agree with you, bad idea.

@oleg-py

This comment has been minimized.

Collaborator

oleg-py commented Feb 3, 2018

@alexandru I made it work for Scala 2.11 / Java 8. Seemed easier 😅 (esp. since CI uses that combination for builds too). WDYT?

/** Convert `CompletableFuture` to [[monix.execution.CancelableFuture]]
*
* If the source is cancelled, returned `Future` will never terminate

This comment has been minimized.

@alexandru

alexandru Feb 9, 2018

Member

Note a .onCancelRaiseError op for CancellableFuture would work for non-terminating futures, even if the source is already cancelled 😉

@alexandru alexandru merged commit 6574c86 into monix:master Feb 9, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
lazy val javaJVM = project.in(file("monix-java"))
.configure(profile)
.dependsOn(executionJVM % "provided->compile; test->test")
.dependsOn(evalJVM % "provided->compile; test->test")

This comment has been minimized.

@alexandru

alexandru Feb 9, 2018

Member

Hope this works 😀

@alexandru

This comment has been minimized.

Member

alexandru commented Feb 9, 2018

Merged. Hope it works well.

Thanks.

@fancywriter

This comment has been minimized.

fancywriter commented Mar 2, 2018

@alexandru @oleg-py
Hi guys! It may be silly question, but what dependency do I have to add to my project to make monix.java.* package available? I didn't find it in README.md...

monix-java of last version 2.3.3 can't be resolved...

@oleg-py

This comment has been minimized.

Collaborator

oleg-py commented Mar 2, 2018

@fancywriter this should be available starting from 3.0.0-M4 (not yet released) and there are currently no plans to backport it to 2.x.

I suggest you to copy relevant parts to your codebase, if you need it ASAP or cannot upgrade.

@alexandru

This comment has been minimized.

Member

alexandru commented Mar 2, 2018

Sorry guys for procrastinating on releasing 3.0.0-M4. The release process is really annoying 😑

It's ready for release though, will do so sometime today.

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