Fix propagation of `ExitCase` in `Resource#{both,combineK}` #3307

armanbilge · 2022-12-06T06:05:14Z

Noticed this while looking into #3227. ExitCases were not being propagated at all to the finalizers.

armanbilge · 2022-12-06T06:09:43Z

tests/shared/src/test/scala/cats/effect/ResourceSpec.scala

+        "left errored, test left" >> ticked { implicit ticker =>
+          var got: ExitCase = null
+          val ex = new Exception
+          val r = Resource.onFinalizeCase(ec => IO { got = ec }) *>
+            Resource.eval(IO.raiseError(ex))
+          r.combineK(Resource.unit).use_ must completeAs(())
+          got mustEqual ExitCase.Succeeded
+        }


This semantic is kind of counter-intuitive. Even though the left Resource errors, ultimately the combineKed Resource is acquired and used successfully. So the left finalizer is passed Succeeded.

This is a by-product of the fact that the left Resource is not released eagerly even if it errors. Maybe this semantic is undesirable?

After mulling this over I think it makes sense and is expected, although I could be wrong (I went back and forth on this in my mind a couple times before settling on the conclusion). It would be consistent with what Resource.both is doing, the reasoning being as follows:

Resource.both requires both Resources to be acquired successfully - if any of them errors out (or gets canceled), then all of them get an ExitCase.Errored (or ExitCase.Canceled respectively), even the one that succeded, because the operation as a whole has failed.

Conversely, Resource.combineK needs any of the combined Resources to be acquired successfully - if just one of them succedes, then all of them (well, the ones that were actually evaluated) get an ExitCase.Succeeded, even the ones that failed, because the operation as a whole has succeded.

The fact that ExitCase from the use operation propagates to all the input Resources, regardless of their own, seems to be aligned as well.

I hope it makes sense. In any case, whichever semantic will be introduced, it could be a good idea to document it in the scaladoc, especially if it seems to be counter-intuitive. 🙂

However, I'm not sure if any sort of consistency between Reource.both and combineK is desired. There's also the case of Resource.race which will release loser eagerly in #3226.

The fact that ExitCase from the use operation propagates to all the input Resources, regardless of their own, seems to be aligned as well.

One more thought regarding this: one could say that in the case of combineK only one Resource is used in the end (similarly to the winner of Resource.race and unlike Resource.both which utilizes all input Resources), so there's no need to propagate ExitCase from use to the one that wasn't. So I guess that could be the reason to maybe release eagerly in combineK. Sorry for the rambling, perhaps I'm not as convinced as I was before. 😄

I feel like this is somewhat similar to the following:

case object TestException extends RuntimeException Resource.raiseError[IO, Unit, Throwable](TestException) .onFinalizeCase(IO.println(_)) .voidError

Do we print out Succeeded here? If so, then this combineK semantic is reasonable. If not, then we're being inconsistent.

The following program doesn't print.

//> using lib "org.typelevel::cats-effect::3.4.2" import cats.effect._ import cats.syntax.all._ object App extends App { case object TestException extends RuntimeException def run = Resource .raiseError[IO, Unit, Throwable](TestException) .onFinalizeCase(IO.println(_)) .voidError .use_ }

It prints for me? Succeeded. Here's an even more interesting example:

(Resource.unit.onFinalizeCase(IO.println(_)) *> Resource.raiseError[IO, Unit, Throwable](TestException)).voidError.use_

Prints Succeeded. Meanwhile:

(Resource.unit.onFinalizeCase(IO.println(_)) *> Resource.raiseError[IO, Unit, Throwable](TestException)).use_

Prints Errored(...).

I think combineK is fine as-is. It's rather weird, but we're at least consistent with the backpropagation of final state.

filipwiech · 2022-12-10T04:23:29Z

tests/shared/src/test/scala/cats/effect/ResourceSpec.scala

+          got mustEqual ExitCase.Errored(ex)
+        }
+
+        "right errored, test left" >> ticked { implicit ticker =>


Would it be a good idea to describe exit case propagation semantics in the scaladoc for the Resource#both? 🙂

Yes, even in two weeks I've forgotten what it was 😂

filipwiech · 2022-12-10T04:28:37Z

tests/shared/src/test/scala/cats/effect/ResourceSpec.scala

+          got mustEqual ExitCase.Succeeded
+        }
+
+        "use canceled, test left" >> ticked { implicit ticker =>


Would it make sense to add cases for "use errored/canceled, test right" (for when left Resource fails to acquire, but right one succedes)? 🙂

Yes it would!

filipwiech · 2022-12-10T10:49:49Z

tests/shared/src/test/scala/cats/effect/ResourceSpec.scala

+          got mustEqual ExitCase.Canceled
+        }
+
+        "right canceled, test left" >> ticked { implicit ticker =>


What would happen if right was canceled and left errored out (and vice versa)? Would they both get ExitCase.Errored? Maybe it could be a good idea to add tests for those cases. 🙂

What would happen if right was canceled and left errored out (and vice versa)?

Depends which happens first. If right cancels first, it's covered by this test. If left errors first it's covered by "left errored, test right"

filipwiech · 2022-12-10T12:40:58Z

tests/shared/src/test/scala/cats/effect/ResourceSpec.scala

+          got mustEqual ExitCase.Succeeded
+        }
+
+        "left errored, test right" >> ticked { implicit ticker =>


In case one of the Resources errors out during acqusition and the other one self-cancels, will the ExitCase of the right Resource propagate/overwrite the result on the left? Could be worth it to add tests for those scenarios. 🙂

If a resource errors out or cancels during acquisition, then it has no exit case because the finalizer never runs.

filipwiech · 2022-12-10T12:43:27Z

tests/shared/src/test/scala/cats/effect/ResourceSpec.scala

+          got mustEqual ExitCase.Errored(ex)
+        }
+
+        "left errored, test left" >> ticked { implicit ticker =>


Would it make sense to add tests for "left canceled, test left/right" too? 🙂

If left cancels, no resources are acquired, so no finalizers are run.

djspiewak

Adding a Request Changes for @filipwiech's suggestions, which are all excellent

armanbilge · 2022-12-23T22:20:54Z

@djspiewak does that mean you think we should change the combineK semantic?

armanbilge added 6 commits December 6, 2022 04:01

Correctly handle exit cases in Resource#both

8d70133

Revert changes to Resource#both

1a37a02

Handle ExitCase in Resource#fold, fix both, combineK

d82fba1

Split into smaller tests

605dccf

Add ExitCase tests for Resource#combineK

d2d27d6

Fix awk wording

9e8d092

armanbilge commented Dec 6, 2022

View reviewed changes

filipwiech reviewed Dec 10, 2022

View reviewed changes

djspiewak requested changes Dec 23, 2022

View reviewed changes

armanbilge added 3 commits December 24, 2022 03:01

Document ExitCase semantics for Resource#both

484a984

Add more test cases for r Resource#combineK

d1f0397

Fix scaladoc linking

be51ea6

djspiewak approved these changes Dec 24, 2022

View reviewed changes

djspiewak merged commit fde936b into typelevel:series/3.4.x Dec 24, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix propagation of `ExitCase` in `Resource#{both,combineK}` #3307

Fix propagation of `ExitCase` in `Resource#{both,combineK}` #3307

armanbilge commented Dec 6, 2022

armanbilge Dec 6, 2022

filipwiech Dec 10, 2022 •

edited

Loading

filipwiech Dec 10, 2022 •

edited

Loading

djspiewak Dec 24, 2022

armanbilge Dec 24, 2022

djspiewak Dec 24, 2022

filipwiech Dec 10, 2022 •

edited

Loading

armanbilge Dec 24, 2022

filipwiech Dec 10, 2022 •

edited

Loading

armanbilge Dec 24, 2022

filipwiech Dec 10, 2022

armanbilge Dec 24, 2022

filipwiech Dec 10, 2022

armanbilge Dec 24, 2022

filipwiech Dec 10, 2022

armanbilge Dec 24, 2022

djspiewak left a comment

armanbilge commented Dec 23, 2022

Fix propagation of ExitCase in Resource#{both,combineK} #3307

Fix propagation of ExitCase in Resource#{both,combineK} #3307

Conversation

armanbilge commented Dec 6, 2022

Choose a reason for hiding this comment

filipwiech Dec 10, 2022 • edited Loading

Choose a reason for hiding this comment

filipwiech Dec 10, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

filipwiech Dec 10, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

filipwiech Dec 10, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

djspiewak left a comment

Choose a reason for hiding this comment

armanbilge commented Dec 23, 2022

Fix propagation of `ExitCase` in `Resource#{both,combineK}` #3307

Fix propagation of `ExitCase` in `Resource#{both,combineK}` #3307

filipwiech Dec 10, 2022 •

edited

Loading

filipwiech Dec 10, 2022 •

edited

Loading

filipwiech Dec 10, 2022 •

edited

Loading

filipwiech Dec 10, 2022 •

edited

Loading