Add a memoizedAcquire method to Resource#3890
Conversation
| Concurrent[Resource[F, *]].memoize[A](self).map { | ||
| case Resource.Eval(fa) => fa | ||
| case unexpected => | ||
| throw new IllegalStateException( | ||
| s"Memoized Resource is not Resource.Eval: $unexpected") | ||
| } |
There was a problem hiding this comment.
Actually, we can avoid this silliness if we go the other way: put the juicy implementation in here, and then memoize can delegate to memoizedAcquire and wrap it up in Resource.eval.
There was a problem hiding this comment.
Also then, the existing memoize tests seem like sufficient coverage to me.
armanbilge
changed the title
memoizedAcquire method to Resource
|
To test this I would write to Refs in your acquire and use, and assert on the state of those refs to check that it's behaving as you expect. I wouldnt worry about concurrent testing since memoize is pulling most of the weight |
|
I don't think we need to add any new tests specifically because of this PR, unless there are existing gaps in the existing To be clear, what I'm proposing is that we refactor so it looks like this: def memoizedAcquire(ra: Resource[F, A]): Resource[F, F[A]] =
/* move existing memoize implementation here */
def memoize(ra: Resource[F, A]): Resource[F, Resource[F, A]] =
memoizedAcquire.map(Resource.eval(_))Then all the existing |
|
Yeah fair enough, with that refactor I would agree that we don't need any new tests |
|
I'm a huge fan of not having to add new tests. I will get to that refactor when/as I have time but if anyone wants to beat me to it, the branch is open for maintainer commits ;) |
| val fa2 = F.uncancelable { poll => | ||
| poll(allocatedCase).flatMap { case (a, r) => release.update(r :: _).as(a) } | ||
| } | ||
| Resource.makeCaseFull[F, F[B]](poll => poll(F.memoize(fa2).map(_.widen))) { (_, exit) => |
There was a problem hiding this comment.
| Resource.makeCaseFull[F, F[B]](poll => poll(F.memoize(fa2).map(_.widen))) { (_, exit) => | |
| Resource.makeCaseFull[F, F[B]](poll => poll(F.memoize(fa2)).map(_.widen)) { (_, exit) => |
Otherwise I think it can get cancelled right after F.memoize(fa2), and I think that would be not good.
There was a problem hiding this comment.
Oh I see, good catch. Probably we should add a test case.
There was a problem hiding this comment.
I will look into adding a test case for this, let's see what I can come up with.
There was a problem hiding this comment.
I tried to write a test for this but failed to find a reliable way to ensure acquisition is canceled right after the call to F.memoize(fa2) and before the end of .map(_,widen). Is there some tool in the testkit that would allow me to do so and that I am not aware of?
|
I don't have the time to update this branch with the changes but I encourage anyone motivated to take over |
|
Hi @Daenyth, I would be interested in taking this over if that's ok. I'd love to see this feature land in the 3.6 release :-) |
|
@satabin Go for it! I think you'll need to pull this branch locally and open a new pull request so you can push to it. Please be sure to refrain from rebasing the commits; we can carry the conversation more easily from this PR to that one if the shas are not rewritten. |
|
I opened #4105 to continue the work on this PR. |
6 participants
Ref #3513
I'm not sure how to unit test this exactly, I'm open to guidance or commits for that