Remove Ref's flatModify.

[mn98]

If the Ref updates then the effect should be run.

[armanbilge]

Thank you!

[filipwiech]

I have to admit that when I've seen the initial issue (#3306) I didn't understand it immediately and it took me a minute to wrap my head around this problem. Maybe it would be nice to put a comment explaining what we're doing here and why (@armanbilge's remark about "the border between completing the modify and beginning the effect" helped). 🙂

[mn98]

To me this was more about the requirement that all operations on Ref need to be atomic. I guess this comment would be helpful to explain how this particular implementation is achieving that. @armanbilge let me know if you'd like me to add it.

[mn98]

I had to fix the formatting anyway, so I took the opportunity to add the additional comments too. :-)

[filipwiech]

Thank you so much, looks great! 🙂

[mn98]

The one CI failure I see here, mentioning Dispatcher, doesn't appear to be related to the changes in this PR.
But please do let me know if there's anything further required on my side for this one.

[mn98]

The one CI failure I see here, mentioning Dispatcher, doesn't appear to be related to the changes in this PR.

I see CI is all clean now 😀

[durban]

I'm not sure "ensure atomicity" is the correct terminology here. At best, it's misleading. What this does is simply that the resulting F[B] will not get canceled immediately after the modify. I mean... what is atomic here? The modify? It is, but that's not specific to this method (it's a general Ref thing). The resulting F[B]? It may, or may not be... the uncancelable certainly doesn't guarantee it...

[mn98]

I'm not sure "ensure atomicity" is the correct terminology here. At best, it's misleading. What this does is simply that the resulting F[B] will not get canceled immediately after the modify. I mean... what is atomic here? The modify? It is, but that's not specific to this method (it's a general Ref thing). The resulting F[B]? It may, or may not be... the uncancelable certainly doesn't guarantee it...

Shall we remove ambiguity in the comment, such as below?
I can change the title of the PR too: "flatModify should run the supplied effect if the Ref updates."

    * The `modify` and the effect are chained within an un-cancelable block, otherwise the border between completing
    * the modify and beginning the effect could result in the effect not being run even when the
    * `Ref` is updated. Of course, the effect itself could still be cancelable.

[armanbilge]

I looked into this more and I think I was wrong about how cancelation works. Unfortunately that means this PR doesn't address the issue.

Basically the property we are relying on is that:

IO.uncancelable(poll => ioa *> poll(iob.uncancelable)) <-> (ioa *> iob).uncancelable

Essentially, we are relying on a guarantee that there will be no cancelation between ioa and iob, or more precisely between the poll(...) and the inner uncancelable.

Unfortunately, that is not the case. Here's a demo program that shows it is still possible to be canceled between ioa and iob.

//> using lib "org.typelevel::cats-effect::3.4.2"

import cats.effect._

object App extends IOApp.Simple {

  // question: is this equivalent to `(ioa *> iob).uncancelable` ?
  def uncancelableProductR(ioa: IO[Unit], iob: IO[Unit]): IO[Unit] =
    IO.uncancelable(poll => ioa *> poll(iob.uncancelable))

  def count = IO.ref(0).flatMap { counter =>
    val addTwo = uncancelableProductR(counter.update(_ + 1), counter.update(_ + 1))
    addTwo.start.flatMap { fiber =>
      fiber.cancel *> counter.get
    }
  } 

  def run =
    (count <* IO.cede)
      // either the fiber is canceled before it starts, or it runs to completion. right?
      .iterateWhile(result => result == 0 || result == 2)
      .flatMap { unexpectedResult =>
        IO.println(s"got $unexpectedResult")
      }

}

output:

got 1

In retrospect it makes sense I suppose. I'm a little disappointed that this is how it works, since it means the responsibility for managing uncancelability falls to users in cases like this. I wonder if it's possible and useful to change this semantic, or too harmful.

[mn98]

That is very interesting, @armanbilge. Aside from changing the semantic, do I understand correctly that our immediate options are:

1. Leave the implementation unchanged and add a comment warning that responsibility for cancellation is on the user.
2. Make the whole operation un-cancellable, so the user has no control over canceling the effect when the Ref updates.

[armanbilge]

@mn98 right, that's a good summary. There is sort of a third option, but it would be highly unusual. We could define something like this:

def flatModify[B, E](f: Poll[F] => A => (A, F[B]))(implicit F: MonadCancel[F, E])

This would be an extension of option (2): we make the operation uncancelable and pass the poll to the user so that they may unmask their action if they wish.

But for now, probably the way forward is improving the documentation as proposed in (1). Maybe this is not unreasonable anyway: the user would still have to remember to make their F[B] action uncancelable if it's more than a single op, so in those cases it would require user intervention anyway.

I think there's a lot places that may have this issue (recently I was looking at Signal implementations in FS2).

[mn98]

But for now, probably the way forward is improving the documentation as proposed in (1). Maybe this is not unreasonable anyway: the user would still have to remember to make their F[B] action uncancelable if it's more than a single op, so in those cases it would require user intervention anyway.

I've made a change to this effect and updated the PR's title accordingly. It all feels a bit underwhelming now but it's been enlightening nevertheless! :-)

[armanbilge]

It all feels a bit underwhelming now but it's been enlightening nevertheless! :-)

Heh, sorry for sending you on that wild goose chase 😅 thanks for all your work on this! If you are looking for a more interesting follow-up—as I mentioned above there are at least a few places in FS2 (and possibly other projects) where we are using modify(...).flatten without uncancelable, that I'm pretty sure we should be. Would be good to audit those :)

[armanbilge]

Fabio, Daniel, and I discussed this PR earlier on Discord today.
https://discord.com/channels/632277896739946517/839263556754472990/1055870961402003578

My current summary opinion is that:

• it's unclear whether the flatModify combinator as currently formulated is "pulling its weight", since its still up to the user to add the appropriate uncancelable boilerplate
• adding a Poll to the signature as I suggested in Remove Ref's flatModify. #3311 (comment) could be an interesting way to make this combinator more useful, but it would be establishing new (potentially confusing) precedent for what our APIs look like.

[mn98]

I caught up on the Discord discussion. Is it preferable to remove this combinator until a decision is taken on the best signature for exposing Poll?

[durban]

@armanbilge I agree on both points. (I was surprised flatModify even exists.) Regarding adding Poll: I think the fact that such a "strange" API is needed shows exactly that flatModify may not be worth adding.

[mn98]

@durban, flatModify only exists because I commented on Discord that I use modify(…).flatten quite often, and wondered if that was good/common practice or it it was poor design choices on my part that led to it. I was assured that it was quite common and that someone should probably add a combinator for it. And so here we all are :-)

[armanbilge]

I think the fact that such a "strange" API is needed shows exactly that flatModify may not be worth adding.

Well, actually I think this is exactly why it's worth: there are some not-so-obvious cancelation issues to think about, which is precisely why having a helpful combinator would bring these to the forefront. The tricky bit of course is what such an API should look like, and Fabio makes a good point about having a select few, well-known region operators.

Basically, by modifying a Ref (as part of a concurrent state machine), the calling fiber is effectively assuming responsibility for going through with the entire action. This is because even if this fiber no longer cares about the result (i.e. it was canceled mid-operation) there may be other fibers waiting on this Ref that must be notified of the modification. I am hardly an expert, but I would guess that scenarios where completing the returned effect is optional (i.e. it can be wrapped in poll) are far more rare than scenarios where you have to complete the effect uncancelably.

---

by modifying a Ref the calling fiber is effectively taking responsibility going through with the entire action

Tangentially, I find this design pattern of "assuming responsibility" interesting. Compare with a more Actor-like pattern, where some other "supervising" entity has full responsibility for this state and running related effects. In this case, even if the external entity initiating the modification is canceled, this does not corrupt the state machine because it is "owned" by its supervisor.

[durban]

@mn98 Sure, that sounds perfectly reasonable, I didn't mean anything by "I was surprised".

@armanbilge I think you convinced me. Cancellation is hard; if there is a way of doing this which helps in it, that is a good thing. (As you've discovered, as it is now, it doesn't really help; it may even harm.)

there may be other fibers waiting on this Ref that must be notified of the modification

Splitting hairs, but this has its limits: even if I think I've woken up a fiber, it might get cancelled before actually waking up, so I can't be sure (e.g., queues and "lost" items). But that is arguably a bug in that fiber. So in general, I agree.

[armanbilge]

I think you convinced me. Cancellation is hard; if there is a way of doing this which helps in it, that is a good thing.

@durban thanks, great you agree :) now just to figure out how 😅

Splitting hairs, but this has its limits:

Yes it does, and I am interested in plugging more of these holes. For example see #3233.

[armanbilge]

I just randomly remembered: other APIs where we pass the Poll to the user include Resource.makeFull (and Stream.bracketFull). So if flatModify exposed a Poll it wouldn't be completely unprecedented.

Edit: this could even suggest an API where flatModify is uncancelable and flatModifyFull passes through the Poll for more fine-grained control

[mn98]

After much debate, both here and on the TypeLevel Discord, it's been decided that this combinator should be removed entirely, owing to the semantics around cancellation. As a separate follow up, as suggested by @SystemFw, we may want to introduce an improved version with a Poll.

[armanbilge]

As a separate follow up, as suggested by @SystemFw, we may want to introduce an improved version with a Poll.

I would be in favor of this, with an uncancelable flatModify and flatModifyFull(poll => ...).

[mn98]

Oops, wrong button!

[mn98]

As a separate follow up, as suggested by @SystemFw, we may want to introduce an improved version with a Poll.

I would be in favor of this, with an uncancelable flatModify and flatModifyFull(poll => ...).

An uncancelable flatModify would be the first commit on this PR, right?

Do you want this PR to become that again, or should we go ahead and merge this one to remove the combinator before re-opening that debate?

[armanbilge]

An uncancelable flatModify would be the first commit on this PR, right?

Well, not quite: it would be completely uncancelable, so it would not wrap the user effect in poll (or anything else).

---

Do you want this PR to become that again, or should we go ahead and merge this one to remove the combinator before re-opening that debate?

I added this PR to the v3.5.0 milestone so we won't forget to resolve this before the release. Basically, let's see what Daniel thinks ;) thanks for all your work and patience!

[mn98]

An uncancelable flatModify would be the first commit on this PR, right?

Well, not quite: it would be completely uncancelable, so it would not wrap the user effect in poll (or anything else).

Do you want this PR to become that again, or should we go ahead and merge this one to remove the combinator before re-opening that debate?
I added this PR to the v3.5.0 milestone so we won't forget to resolve this before the release. Basically, let's see what Daniel thinks ;)

Ok, makes sense.

thanks for all your work and patience!

No problem at all, contributing in some small capacity is a great way to learn :-)

[djspiewak]

@mn98 Thanks for sticking with this! You've honestly done some really good work here.