-
Notifications
You must be signed in to change notification settings - Fork 533
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
Fix propagation of ExitCase
in Resource#{both,combineK}
#3307
Fix propagation of ExitCase
in Resource#{both,combineK}
#3307
Conversation
"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 | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This semantic is kind of counter-intuitive. Even though the left Resource
errors, ultimately the combineK
ed 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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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_
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
got mustEqual ExitCase.Errored(ex) | ||
} | ||
|
||
"right errored, test left" >> ticked { implicit ticker => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be a good idea to describe exit case propagation semantics in the scaladoc for the Resource#both
? 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, even in two weeks I've forgotten what it was 😂
got mustEqual ExitCase.Succeeded | ||
} | ||
|
||
"use canceled, test left" >> ticked { implicit ticker => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense to add cases for "use errored/canceled, test right" (for when left Resource fails to acquire, but right one succedes)? 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it would!
got mustEqual ExitCase.Canceled | ||
} | ||
|
||
"right canceled, test left" >> ticked { implicit ticker => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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"
got mustEqual ExitCase.Succeeded | ||
} | ||
|
||
"left errored, test right" >> ticked { implicit ticker => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If a resource errors out or cancels during acquisition, then it has no exit case because the finalizer never runs.
got mustEqual ExitCase.Errored(ex) | ||
} | ||
|
||
"left errored, test left" >> ticked { implicit ticker => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense to add tests for "left canceled, test left/right" too? 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If left cancels, no resources are acquired, so no finalizers are run.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding a Request Changes for @filipwiech's suggestions, which are all excellent
@djspiewak does that mean you think we should change the |
Noticed this while looking into #3227.
ExitCase
s were not being propagated at all to the finalizers.