Skip to content

Propagate Java thread interruption in Dispatcher#unsafeRunSync#4167

Merged
djspiewak merged 6 commits intotypelevel:series/3.5.xfrom
kamilkloch:unsafe-run-sync-interrupt
Nov 21, 2024
Merged

Propagate Java thread interruption in Dispatcher#unsafeRunSync#4167
djspiewak merged 6 commits intotypelevel:series/3.5.xfrom
kamilkloch:unsafe-run-sync-interrupt

Conversation

@kamilkloch
Copy link
Copy Markdown
Contributor

@kamilkloch kamilkloch commented Nov 12, 2024 •
edited
Loading

Closes #4166.

armanbilge
armanbilge reviewed Nov 12, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@armanbilge armanbilge left a comment

Choose a reason for hiding this comment

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

We should also make a similar change for IO#unsafeRunSync.

@kamilkloch
Copy link
Copy Markdown
Contributor Author

kamilkloch commented Nov 12, 2024

We should also make a similar change for IO#unsafeRunSync.

I got a bit lost trying to comprehend the nuanced

cats-effect/core/jvm/src/main/scala/cats/effect/IOPlatform.scala

Line 70 in 725a4cd

final def unsafeRunTimed(limit: FiniteDuration)(

@armanbilge
Copy link
Copy Markdown
Member

armanbilge commented Nov 13, 2024

I got a bit lost trying to comprehend the nuanced

Actually, maybe let's hold off on that change. I forgot that the specification for this method is a bit unusual.

cats-effect/core/jvm/src/main/scala/cats/effect/IOPlatform.scala

Lines 45 to 48 in 725a4cd

* Similar to `unsafeRunSync`, except with a bounded blocking duration when awaiting
* asynchronous results. As soon as an async blocking limit is hit, evaluation ''immediately''
* 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.

But if we did decide to move forward ...

Details

You can factor out a package-private version of unsafeRunAsync that returns the Fiber.

cats-effect/core/shared/src/main/scala/cats/effect/IO.scala

Lines 979 to 992 in 725a4cd

def unsafeRunAsync(cb: Either[Throwable, A] => Unit)(
implicit runtime: unsafe.IORuntime): Unit = {
unsafeRunFiber(
cb(Left(new CancellationException("The fiber was canceled"))),
t => {
if (!NonFatal(t)) {
t.printStackTrace()
}
cb(Left(t))
},
a => cb(Right(a))
)
()
}

Then, you can use that for cancelation in unsafeRunTimed.

cats-effect/core/jvm/src/main/scala/cats/effect/IOPlatform.scala

Lines 79 to 85 in 725a4cd

try {
val result = blocking(queue.poll(limit.toNanos, TimeUnit.NANOSECONDS))
if (result eq null) None else result.fold(throw _, Some(_))
} catch {
case _: InterruptedException =>
None
}

@kamilkloch
Copy link
Copy Markdown
Contributor Author

kamilkloch commented Nov 13, 2024

Actually, maybe let's hold off on that change. I forgot that the specification for this method is a bit unusual.

If only you are ok with the resulting discrepancy in sematics between Dispatcher#unsafeRunSync and IO#unsafeRunSync, I would hold off as well. (Actually, for unsafeRunTimed the semantics is already different)

@armanbilge armanbilge changed the title Propagate Java thread interruption in unsafeRunSync. Fixes #4166. Propagate Java thread interruption in Dispatcher#unsafeRunSync Nov 14, 2024
armanbilge
armanbilge approved these changes Nov 14, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@armanbilge armanbilge left a comment

Choose a reason for hiding this comment

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

thanks!

@armanbilge armanbilge added this to the v3.6.0 milestone Nov 21, 2024
@djspiewak djspiewak merged commit 58a54e8 into typelevel:series/3.5.x Nov 21, 2024
@armanbilge armanbilge mentioned this pull request Feb 25, 2025
Merged
@armanbilge armanbilge mentioned this pull request Mar 10, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@djspiewak djspiewak djspiewak approved these changes

@armanbilge armanbilge armanbilge approved these changes

Assignees

No one assigned

Labels

🍄 enhancement

Projects

None yet

Milestone

v3.6.next

Development

Successfully merging this pull request may close these issues.

3 participants

@kamilkloch @armanbilge @djspiewak