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

Create UnsafeNonFatal for use in the fiber runtime #4263

Open
wants to merge 6 commits into
base: series/3.6.x
Choose a base branch
from

Conversation

kapunga
Copy link
Contributor

@kapunga kapunga commented Jan 31, 2025

Creates a custom NonFatal type for use in the fiber runtime. This is needed because while application code generally shouldn't catch InterruptedException, the fiber runtime is responsible for dealing with interrupts gracefully.

Initially I considered naming this RuntimeNonFatal, however I think UnsafeNonFatal makes it really clear that you shouldn't use it without knowing what you are doing.

Will resolve #4254

@armanbilge
Copy link
Member

Thanks for the PR!! Sorry, would you mind re-targeting to the series/3.6.x branch so we can get it into the 3.6.0 release? 🙏

@kapunga kapunga force-pushed the 4254-custom-non-fatal branch from 569d1c9 to ee6f0b8 Compare February 1, 2025 03:51
@kapunga kapunga changed the base branch from series/3.x to series/3.6.x February 1, 2025 03:52
@kapunga
Copy link
Contributor Author

kapunga commented Feb 1, 2025

Thanks for the PR!! Sorry, would you mind re-targeting to the series/3.6.x branch so we can get it into the 3.6.0 release? 🙏

Done.

@kapunga kapunga force-pushed the 4254-custom-non-fatal branch from 3970f74 to 58a9c5d Compare February 1, 2025 04:15
@kapunga kapunga force-pushed the 4254-custom-non-fatal branch from 0a76d0f to dd85c6f Compare February 9, 2025 15:44
@kapunga kapunga force-pushed the 4254-custom-non-fatal branch from 7d6dc58 to 83fecb9 Compare February 11, 2025 02:46
@kapunga kapunga changed the title [WIP] Create UnsafeNonFatal for use in the fiber runtime Create UnsafeNonFatal for use in the fiber runtime Feb 11, 2025
@kapunga kapunga marked this pull request as ready for review February 11, 2025 03:18
@kapunga kapunga requested a review from armanbilge February 11, 2025 03:18
@kapunga
Copy link
Contributor Author

kapunga commented Feb 11, 2025

Note, there is a reference to NonFatal in cats.effect.std.SecureRandomCompanionPlatform and another in cats.effect.kernel.testkit.TestContext that can't access UnsafeNonFatal.

@kapunga kapunga requested a review from durban February 11, 2025 03:25
@durban
Copy link
Contributor

durban commented Feb 12, 2025

Note, there is a reference to NonFatal in cats.effect.std.SecureRandomCompanionPlatform and another in cats.effect.kernel.testkit.TestContext that can't access UnsafeNonFatal.

Why can't they access it?

@armanbilge
Copy link
Member

Why can't they access it?

Because UnsafeNonFatal is in core, and those modules don't depend on core.

@CJSmith-0141
Copy link

Note, there is a reference to NonFatal in cats.effect.std.SecureRandomCompanionPlatform and another in cats.effect.kernel.testkit.TestContext that can't access UnsafeNonFatal.

Why can't they access it?

Looks like testkit and std both depend on kernel and not core.

@durban
Copy link
Contributor

durban commented Feb 12, 2025

Ah, of course... could we put this in kernel? It might be ugly, but it's private, so whatever...

Also, I don't really like the Unsafe prefix, but I don't really have a better idea, and it's probably good that it has a different name than the one in the stdlib, so... 🤷‍♂️

@CJSmith-0141
Copy link

Ah, of course... could we put this in kernel? It might be ugly, but it's private, so whatever...

Also, I don't really like the Unsafe prefix, but I don't really have a better idea, and it's probably good that it has a different name than the one in the stdlib, so... 🤷‍♂️

What about EffectRuntimeNonFatal ?

@armanbilge
Copy link
Member

Looks like testkit and std both depend on kernel

testkit depends on core, but kernel-testkit does not.

Ah, of course... could we put this in kernel? It might be ugly, but it's private, so whatever...

Do we really need it in the other places? Fatal exceptions are a runtime concern, and all the fatal error handling machinery is in core, because that is where the runtime lives.

Meanwhile, if we pursue this thread we actually should consider moving it to Cats, which also uses NonFatal.

@kapunga
Copy link
Contributor Author

kapunga commented Feb 12, 2025

Ah, of course... could we put this in kernel? It might be ugly, but it's private, so whatever...
Also, I don't really like the Unsafe prefix, but I don't really have a better idea, and it's probably good that it has a different name than the one in the stdlib, so... 🤷‍♂️

What about EffectRuntimeNonFatal ?

I started withRuntimeNonFatal so I could do that. I thought the name should be different so it's easily differentiable from regular NonFatal, but coming up with another name was hard...

@kapunga
Copy link
Contributor Author

kapunga commented Feb 12, 2025

Do we really need it in the other places? Fatal exceptions are a runtime concern, and all the fatal error handling machinery is in core, because that is where the runtime lives.

Meanwhile, if we pursue this thread we actually should consider moving it to Cats, which also uses NonFatal.

I agree that it shouldn't be used elsewhere. In my experience, handling InterruptedException is usually only a concern if you are directly handling threads, so I don't think it should be caught anywhere outside of the fiber runtime.

durban
durban previously approved these changes Feb 12, 2025
Copy link
Contributor

@durban durban left a comment

Choose a reason for hiding this comment

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

Yeah ok, fair enough. I'll stop nitpicking now... :-)

UnsafeNonFatal is also fine, I was just hoping someone'll have a better idea 🤷‍♂️

@armanbilge armanbilge changed the title Create UnsafeNonFatal for use in the fiber runtime Create UnsafeNonFatal for use in the fiber runtime Feb 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use our own NonFatal exception classifier
4 participants