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

CancelableFuture #1655

Merged
merged 10 commits into from Sep 30, 2019
Merged

CancelableFuture #1655

merged 10 commits into from Sep 30, 2019

Conversation

fsvehla
Copy link
Contributor

@fsvehla fsvehla commented Sep 12, 2019

Closes #1627

Delegates calls to a Future that it wraps. I’ve found a lightweight way to support Scala 2.11 as well.

@iravid
Copy link
Member

iravid commented Sep 12, 2019

Seeing that that this is an impure construct meant mainly for interop with Future-based programs, it appears that cancel would be more useful as an impure operation.

@iravid
Copy link
Member

iravid commented Sep 12, 2019

I'm also wondering about cancel returning the Exit[E, A], and about E appearing at all in the trait.

@fsvehla
Copy link
Contributor Author

fsvehla commented Sep 12, 2019

My initial idea was def cancel: Unit and no E in the trait (see some discussion on the original ticket).

Since we return the CancelableFuture also from Fiber / Zio, I wouldn’t know how to get a Runtime to run the cancellation action with. We also couldn’t use it under Scala.JS.

@iravid
Copy link
Member

iravid commented Sep 12, 2019

You can access the runtime at any point using ZIO.runtime[R]. But Scala.JS is a good point. Maybe we should only offer asynchronous cancellation?

@fsvehla
Copy link
Contributor Author

fsvehla commented Sep 12, 2019

In order to run the UIO[Future] returned from toFuture a runtime must be available anyway at the call site.

About ZIO.runtime - I still wouldn’t be able to return a synchronous result because it returns a UIO[Runtime] – am I missing something?

@iravid
Copy link
Member

iravid commented Sep 12, 2019

@fsvehla

final def toFutureWith(f: E => Throwable): UIO[CancelableFuture[E, A]] = 
  ZIO.runtime[Any].flatMap { runtime =>
    // construct CancelableFuture here, using the runtime
  }

@fsvehla
Copy link
Contributor Author

fsvehla commented Sep 12, 2019

test_212_jdk11_jvm failure is unrelated.

@fsvehla fsvehla marked this pull request as ready for review September 12, 2019 17:27
@fsvehla fsvehla changed the title Introcude CancelableFuture CancelableFuture Sep 12, 2019

object CancelableFutureSpec
extends ZIOBaseSpec(
suite("CancelableFutureSpec")(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love the new ZIO Test!

@fsvehla
Copy link
Contributor Author

fsvehla commented Sep 12, 2019

@iravid I’ve understood now – it didn’t occur to me to refer to the Runtime when it is created.

@dpennell
Copy link
Contributor

Is there anything I can do to help with this?

@fsvehla
Copy link
Contributor Author

fsvehla commented Sep 20, 2019

It will be ready soon. I am at the hackathon and will wrap it up tomorrow

@dpennell
Copy link
Contributor

@fsvehla I'm jealous - wish I could be there!

@iravid iravid added this to In progress in Warsaw Hackathon via automation Sep 21, 2019
@fsvehla
Copy link
Contributor Author

fsvehla commented Sep 21, 2019

addressed all comments.


package zio

trait Cancelable[+E, +A] {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is only used in Cancelable, right? In that case, I'd inline it completely so we can delete the class.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jdegoes You were actually correct, there is java.util.Cancelable

def cancel(mayInterruptIfRunning: Boolean): Boolean
def isCancelled(): Boolean
def isDone(): Boolean

Let’s use that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That JCP was never accepted,

@fsvehla
Copy link
Contributor Author

fsvehla commented Sep 21, 2019

Inlined.

extends Future[A]
with FutureTransformCompat[A] {

def cancel: Exit[E, A]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this could return another Future instead of blocking the current thread?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no strong opinion here, but see
#1655 (comment)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, if we go back to the original use case:

We've started introducing zio at the edges of our system. This results in having multiple unsafeRunToFutures running in different actors. I'm looking for advice on terminating these from outside of zio (e.g. when the parent actor is shutting down).

I'm fairly sure introducing a potentially never-ending blocking operation inside an akka actor is a big no-no. Note that Future/scala.Promise is also impure and it's already running the moment it's returned, so it's no less ergonomic to use it in impure code. In fact, I'd expect that most users will not care about the Exit return AND might not want to wait until the fiber is interrupted, in which case the default run-and-forget behavior of cancel without Await is preferrable.

Note that there would be no way for a user to achieve run-and-forget without blocking a thread if we commit to a synchronous API.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍, Kai. My initial idea was to return an UIO[Exit[E, A]], because the runtime would have been around anyway at the call site where the CancelableFuture is created. Returning a Future sounds like a good compromise

import scala.concurrent.duration.Duration
import scala.util.Try

private[zio] abstract class CancelableFuture[+E, +A](val future: Future[A])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might want to make it public so that users can use the additional cancellation ability!

@fsvehla
Copy link
Contributor Author

fsvehla commented Sep 28, 2019

Addressed comments

Copy link
Member

@neko-kai neko-kai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

new CancelableFuture[E, A](p.future) {
def cancel: Future[Exit[E, A]] = runtime.unsafeRunToFuture(interrupt)
}
} <* self.await
.flatMap[Any, Nothing, p.type](_.foldM[Any, Nothing, p.type](failure, success))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if type annotations could be avoided if concurrent.Promise[A] is used in success/failure returns instead of p.type

@jdegoes
Copy link
Member

jdegoes commented Sep 30, 2019

@fsvehla Thank you for all your work on this feature, and congratulations on your first ZIO contribution!

giphy

@jdegoes jdegoes merged commit aee5d76 into zio:master Sep 30, 2019
Warsaw Hackathon automation moved this from In progress to Done Sep 30, 2019
@dpennell
Copy link
Contributor

ZIO team - thanks for addressing this!

@fsvehla fsvehla deleted the ferdinand/cancelable-future branch October 8, 2019 14:16
Twizty pushed a commit to Twizty/zio that referenced this pull request Nov 13, 2019
* Introduce CancelableFuture

* Fix race in CancelableFutureSpec

* Impure cancel

* Inline Cancelable

* Make CancelableFuture public

* cancel returns a Future

* Format

* Fix mdoc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Proposal: CancelableFuture
5 participants