Skip to content
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

parallel finalization in Concurrent[Resource]#both #1689

Merged
merged 2 commits into from
Feb 20, 2021
Merged

parallel finalization in Concurrent[Resource]#both #1689

merged 2 commits into from
Feb 20, 2021

Conversation

patseev
Copy link
Contributor

@patseev patseev commented Feb 15, 2021

Fixed the inconsistency between Resource#both and Async[Resource]#both.
Now both methods run finalizers in parallel.
Related discussion: #1576

@patseev patseev changed the title parallel release in Async[Resource] parallel finalization in Async[Resource]#both Feb 15, 2021
@@ -1119,6 +1119,9 @@ abstract private[effect] class ResourceAsync[F[_]]
override def never[A]: Resource[F, A] =
Resource.eval(F.never[A])

override def both[A, B](fa: Resource[F, A], fb: Resource[F, B]): Resource[F, (A, B)] =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another thought... how does race handle finalizers? The default implementations of both and bothOutcome use racePair, so if we could implement that for Resource, we should get "the right thing" in both/bothOutcome/race for free. @SystemFw @djspiewak what do you think?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the problem is that racePair doesn't have any special understanding of the way in which finalizers need to be composed. Ultimately, the "for free" parallelism here, or lack thereof, comes down to the semantics of ap, which underlies tupled, which underlies both. I'm actually relatively certain that it's impossible to get the "right thing" for free here, and it just has to be a bespoke special case. That's okay though so long as we're viewing it as a performance optimization and not as a semantic guarantee.

@patseev patseev changed the title parallel finalization in Async[Resource]#both parallel finalization in Concurrent[Resource]#both Feb 16, 2021
@djspiewak djspiewak merged commit 7ff3141 into typelevel:series/3.x Feb 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants