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

Dispatcher#unsafeRunTimed should also cancel() if Await.result is interrupted #3967

Open
armanbilge opened this issue Jan 22, 2024 · 7 comments

Comments

@armanbilge
Copy link
Member

armanbilge commented Jan 22, 2024

i.e. if it throws an InterruptedException.

def unsafeRunTimed[A](fa: F[A], timeout: Duration): A = {
val (fut, cancel) = unsafeToFutureCancelable(fa)
try Await.result(fut, timeout)
catch {
case t: TimeoutException =>
cancel()
throw t
}
}

@djspiewak
Copy link
Member

This is a good idea. We should probably also do it in IO. unsafeRunTimed is a bit bizarre because it's the only flavor of cancelation which doesn't backpressure and instead just yolo-disconnects from the computation. We need to keep that semantic (because it is designed to do precisely that), but this doesn't mean we shouldn't at least try to queue up the cancelation.

@armanbilge
Copy link
Member Author

armanbilge commented Jan 22, 2024

We should probably also do it in IO

I thought so, but it would be changing the explicitly documented API.

* aborts and `None` is returned. Note that this does not run finalizers, which makes it quite
* different (and less safe) than other mechanisms for limiting evaluation time.

For the record, I can't think why we wouldn't want to improve it 😅

@djspiewak
Copy link
Member

Yeah I'm in favor of improving both.

@kotoji
Copy link

kotoji commented Feb 12, 2024

@armanbilge @djspiewak
I'm a beginner, and I think it might take me some time, but I'd like to give it a try.
If it's okay with you, could you please assign the issue to me?

@djspiewak
Copy link
Member

It's all yours! Do you feel like you have a good idea on how to start? Feel free to ask questions here or in Discord (in the cats-effect-dev channel) and we'll be happy to help.

@kotoji
Copy link

kotoji commented Feb 12, 2024

@djspiewak
Thank you very much. I'm planning to start by doing some research to understand the content of the issue better. I might ask some basic questions, but I appreciate your support.

@kotoji
Copy link

kotoji commented Feb 25, 2024

@djspiewak
I would like to confirm my understanding of the approach for this issue.

The main content of this issue, as I understand it, is that we want to perform a cancel() operation when an InterruptedException is issued in the following places:

  1. DispatcherPlatform#unsafeRunTimed
  2. IOPlatform#unsafeRunTimed

Next, I will express my thoughts on how we should address this in both (1) and (2).

For (1), my thought is that we should handle an InterruptedException in the same way we do for a TimeoutException, by performing a cancel().

For (2), based on my interpretation of the following comment, #3967 (comment)

I thought the approach might be something like this:

  • In the case of a timeout => yolo-disconnected (current implementation)
  • In the case of being interrupted => cancel()

I'm not sure how to proceed with this, but I would like to confirm if my understanding is correct.

Or perhaps, even in the case of a timeout, is it acceptable to perform a cancel() instead of just being yolo-disconnected? In this case, I believe using unsafeToFutureCancelable() like in DispatcherPlatform#unsafeRunTimed seems to be a viable approach.

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

No branches or pull requests

3 participants