search

typelevel / cats-effect

The pure asynchronous runtime for Scala
https://typelevel.org/cats-effect/
Apache License 2.0
2.03k stars 520 forks source link

better handling of callbacks that might throw in `CallbackStack` #3989

Open samspills opened 9 months ago

samspills commented 9 months ago

Capturing @durban's question here: https://github.com/typelevel/cats-effect/pull/3973#issuecomment-1913140555 And also @armanbilge: https://github.com/typelevel/cats-effect/pull/3943#discussion_r1451633273

I think we're all agreed that the remaining callbacks shouldn't just be abandoned, and when talking it through with Arman he had a proposed refactor:

if CallbackStack encounters a throwing callback, it can install the current/next node as the head in the finally and then let the throw continue this leaves it up to the caller to decide what they want to do if they're cool with the exception and want to invoke the rest of the callbacks, they can just call apply again if they want to tear down, maybe we can bring back clear for this

djspiewak commented 9 months ago

We should probably be pretty careful here. Any performance hit at all as a consequence of this is not worth it, since we know a priori that none of our internal callbacks will throw, and CallbackStack is strictly internal. If we can do it without any performance hit though, it would be nice to not make unnecessary assumptions.