-
Notifications
You must be signed in to change notification settings - Fork 423
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
Adjust Async cancellation to CE 3.5 #2897
Conversation
} | ||
} | ||
}) | ||
Some(F.void(F.delay { javaFuture.cancel(true) })) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't we wait for the future, but then wait for its result, to properly implement cancellation integration?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean call the cancel
but still block until the future completes? I think the finalizer should finish as soon as it finishes its logic. Take a look at this example from The Async
documentation:
def fromCompletableFuture[A](fut: F[CompletableFuture[A]]): F[A] =
flatMap(fut) { cf =>
async[A] { cb =>
delay {
//Invoke the callback with the result of the completable future
val stage = cf.handle[Unit] {
case (a, null) => cb(Right(a))
case (_, e) => cb(Left(e))
}
//Cancel the completable future if the fiber is canceled
Some(void(delay(stage.cancel(false))))
}
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought that the idea in cats-effect is for the finalizers to block until the effect finishes (after cancellation or not). But the example indeed suggests otherwise.
... and reading https://github.com/typelevel/cats-effect/releases/tag/v3.5.0 your code is correct :)
private[cats] def fromFuture[F[_]: Async, A](f: => Future[A]): F[A] = Async[F].async { cb => | ||
Sync[F].delay { | ||
val fut = f.onSuccess(f => cb(Right(f))).onFailure(e => cb(Left(e))) | ||
Some(Sync[F].delay(fut.raise(new InterruptedException("Fiber canceled"))).void) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
aren't twitter futures cancellable? (it's a distinct operation from raise
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess it was called cancel()
in older versions, but now it's this raise
method https://github.com/twitter/util#future-interrupts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah ok thanks :)
cb(Left(handler.cause())) | ||
} | ||
}) | ||
Some(().pure[F]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isn't this fake cancellation logic - shouldn't we wait for f
to complete?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we want to wait for f
to complete then we should return None
here, which basically means leaving async_
as before. The difference is that:
- in older CE (<3.5) canceling such a fiber would not wait for the future and finish immediately (leaving a leaked future)
- in CE 3.5 this IO is uncancellable, leading to potential deadlocks
If there's no way to cancel underlying future, we need to choose between keeping the previous behavior (canceling the outer fiber and leaving a leaked future) and agreeing to the new uncancellable behavior. I chose the first solution to keep consistent with previous behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, well I guess if we can't do anything, we can't :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Fixes #2892
The crucial change in CE 3.5 is that now
async_
is uncancellable, which may lead to deadlocks. It also affectsAsync[F].fromFuture
which relies on it underneath, as well asasync
, which becomes uncancellable when returning aNone
.This PR revises our usages of
async
and friends in order to add proper explicit handling for cancellation where needed.ArmeriaCatsServerOptions[F]
Creating and deleting temp files for Armeria is now converted to
async
with an attempt to cancel the underlying Java Future.CatsFutureConversion
now uses CE'sfromFutureCancelable
, with empty cancellation logic. The underlying Future cannot be canceled, but at least we remain consistent with previous behavior - the fiber that awaits for async callback can be canceled, preventing deadlocks.F
usingasync
, plus a cancellation callback which tries to interrupt the underlying Finatra Future (future.raise(...)
)async
, with a no-op cancellation logic, but at least it should keep previous behavior and avoid deadlocks.