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

Decide what we're going to do with Resource#parZip #1576

Closed
djspiewak opened this issue Jan 2, 2021 · 10 comments
Closed

Decide what we're going to do with Resource#parZip #1576

djspiewak opened this issue Jan 2, 2021 · 10 comments
Milestone

Comments

@djspiewak
Copy link
Member

It's somewhat redundant with both (defined in Concurrent) now, though it is probably slightly faster. The real problem though is that parZip is actually inconsistent with both in that it runs finalizers in parallel, whereas both runs them sequentially.

@djspiewak djspiewak added the CE 3 label Jan 2, 2021
@Daenyth
Copy link
Contributor

Daenyth commented Jan 2, 2021

Why does both run finalizers in sequence? Can that be fixed?

@SystemFw
Copy link
Contributor

SystemFw commented Jan 3, 2021

I kinda would have expected the Parallel newtypes to be CommutativeApplicative, and that, for Resource, order of finalisation is part of equivalence, which make sequential release on both weird.

Validated breaks the CommutativeApplicative assumption admittedly , but I think not having resources released in parallel actually has practical consequences and breaks the principle of least surprise

@djspiewak
Copy link
Member Author

I agree with the commutative applicative argument. It's also worth noting that Validated does work here so long as the inner E forms a commutative monoid. But at any rate…

What I don't see is how to make this happen with Resource without bespoke overrides, which make me nervous in this case. The problem is that we flatMap inside Fiber#joinWith, and we don't have any knowledge of the other fibers when we do so. Even if we did have knowledge of the other fibers, it's not at all clear to me that we could get the finalizers to run in parallel with Resource specifically.

What is it about Resource in particular which gives rise to this property? In abstract, I mean. Are we not likely to see other effects with the same issue in Parallel?

@djspiewak
Copy link
Member Author

In light of everything with EitherT, I'm strongly inclined to say that parZip (now both) should just have the same semantics as the Spawn[Resource[F, *]].both implementation. Which is to say, sequential finalizers. Any strong objections?

@SystemFw
Copy link
Contributor

Which is to say, sequential finalizers. Any strong objections?

it still sounds very very weird to me

@djspiewak
Copy link
Member Author

it still sounds very very weird to me

I agree in a vacuum that parallel finalization makes sense, but I don't see a way that it can be derived from the more primitive semantics without totally special-casing this one specific scenario. It's basically like the whole Parallel[EitherT[F, E thing: one set of semantics arises naturally from base primitives, while the other set of semantics requires a bespoke specialized interpretation of the meaning of ap in this context, which seems like a red flag.

@SystemFw
Copy link
Contributor

SystemFw commented Feb 12, 2021

there is another scarier possibility, which is that the primitive semantics of Async[Resource ultimately don't make sense, and we're looking at the first example of why.
I feel like in EitherT the balance is different, because in that case the main culprit is trying to take a syntactic convenience (Parallel) and building a tower of abstraction over it, and when combined with a cube of possibilities (concurrent/sequential, short-circuiting/accumulation) over a transformer, it crumbled.

In this case, not only we have the specific vs general, but also the fact that Resource.Par being commutative is really the only meaningful semantics, and having parZip introduce a left-to-right order, totally arbitrarily, and only for release, is very weird in isolation, not in combination with another Parallel.

Also, more worryingly, this is happening essentially at the first layer we are exploiting Async[Resource]: parZip is basically the only operation that isn't MonadError that is actually used, and the derived semantics don't work. The other example we tried was memoize, and the derived semantics didn't work. The semantics for join also didn't naturally fall out, but required lots of thinking. Hopefully you see where I'm getting at (with sadness). Imagine when this gets actually stressed.

Also, we don't generally propose abstracting over F[_[_], _], so the main benefit of Async[Resource is deriving combinators. If those derived combinators are worse than what we have, I'm not sure the balance is working out.

@djspiewak
Copy link
Member Author

In this case, not only we have the specific vs general, but also the fact that Resource.Par being commutative is really the only meaningful semantics

Except literally no other Parallel is commutative! Easy example:

(IO.raiseError(e1), IO.raiseError(e2)).parTupled

Commutativity just isn't a guarantee we have in this space. It's roughly intuitive, but it's also just not achievable unless the underlying Applicative is itself commutative.

And this is the crux of the issue. We have an intuition that Parallel for some type should map to a commutative applicative, but the Cats Effect-derived semantics show us that the actual semantics of the mapped applicative are dependent on the underlying primary applicative (note that both's semantics here basically boil down to tupled), which is generally not commutative.

So basically what I'm saying is that intuition is what's wrong, and Cats Effect is showing us that. Parallel, as you pointed out, is not a particularly well defined abstraction. It's fairly ad hoc mechanism, whereas Cats Effect's Concurrent is very carefully and orthogonally defined from first principles, which gives me a lot more confidence in it. It's also intuitive to me that the commutativity of the target applicative is always going to be dependent on the commutativity of the source applicative.

@SystemFw
Copy link
Contributor

SystemFw commented Feb 12, 2021

(IO.raiseError(e1), IO.raiseError(e2)).parTupled

this is commutative though, no? (although you are unlikely to observe it), but I think you have a strong argument, and we can discard commutativity from the discussion (e.g Validated).

However, I think we both agree that concurrent release is preferable if you think about Resource without the async instance , both from first principles and in practice: lots of things have heavy releases that you want to parallelise, e.g skunk use Resource for transactions. Now: do you think is reasonable to say parZip (parMapN) is by far the most used operations that isn't MonadError (this is a question, not a claim in rhetorical form). If it is, the addition of Async[Resource] (or the decision to not override it) makes the most common operation worse, and it doesn't add that much at a practical level. I hugely care about this because I think Resource is one of the winning arguments in a cats-effect vs "normal" scala debate, and any degradation of its qualities should be taken extremely seriously.

In other words: many more users will care about Resource.both not closing in parallel, than about the fact that Async[Resource] exists (or even more subtly, that both is overriden, I although I agree that it's a smell)

@djspiewak
Copy link
Member Author

If we view the parallel release semantics as an optimization and implementation detail of a very common case, rather than as something that's fundamentally dictated by the model, then I'm completely okay with it. :-) That would imply that what I mentioned on #1652 was slightly backwards: we should override both on the Async[Resource] instance to point to the concrete both on Resource itself (race can still delegate to the generic implementation).

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

No branches or pull requests

3 participants