cats-effect icon indicating copy to clipboard operation
cats-effect copied to clipboard

Published 20 hours ago • verified publisher icon typelevel

Add a `memoizedAcquire` method to `Resource`

Open satabin opened this issue 1 year ago • 1 comments
trafficstars

As mentioned in https://github.com/typelevel/cats-effect/pull/3890#issuecomment-2227063576, this is the PR taking over #3890 to add a specialized version of memoized for Resource.

Ref #3513

satabin avatar Jul 18 '24 13:07 satabin

I did not forget about this PR. Testing it is currently blocked by the PureConc finalizer problem (see #3430). As soon as there is a fix, I will design a test using it.

satabin avatar Sep 17 '24 08:09 satabin

Sadly I think this one might be really blocked on the PureConc stuff. It's hard to be totally comfortable with this absent tests.

djspiewak avatar Nov 21 '24 20:11 djspiewak

To refresh my memory: the reason we couldn't write the test in IO (using the test runtime) is because I think that IO actually doesn't reproduce this bug thanks to the optimization introduced in https://github.com/typelevel/cats-effect/pull/1742. Basically, the map is able to execute immediately without requiring an additional iteration of the runloop and thus skips the cancelation check.

armanbilge avatar Nov 21 '24 21:11 armanbilge

It's hard to be totally comfortable with this absent tests.

Also point of clarification, there are several tests for memoizedAcquire (which is just a refactoring of memoize). During refactoring I introduced a bug, which Daniel U caught in review, and this test is intended to catch that.

armanbilge avatar Nov 24 '24 15:11 armanbilge

It's hard to be totally comfortable with this absent tests.

Also point of clarification, there are several tests for memoizedAcquire (which is just a refactoring of memoize). During refactoring I introduced a bug, which Daniel U caught in review, and this test is intended to catch that.

Yes, the currently committed test does not reflect this, it is still an attempt I wrote before noticing I couldn't reproduce it with IO. I will update the test with what I think should work, once the PureConc behavior is fixed.

satabin avatar Nov 25 '24 08:11 satabin

Needs prePR

djspiewak avatar Nov 26 '24 22:11 djspiewak

Needs prePR

Yes, I should have time to work on this this weekend. Regarding the test I wanted to add to check that acquisition cancellation does not occur after the resource has been actually acquired and before it is put in the release list. Should I just not add it, or add and ignore it, until PureConc is fixed? What do you think is the best approach?

satabin avatar Nov 27 '24 07:11 satabin

Regarding the test I wanted to add to check that acquisition cancellation does not occur after the resource has been actually acquired and before it is put in the release list. Should I just not add it, or add and ignore it, until PureConc is fixed? What do you think is the best approach?

Ideally add it and then mark pendingUntilFixed.

djspiewak avatar Nov 27 '24 17:11 djspiewak