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

Add MVar.modify? #771

Closed
scf37 opened this issue Feb 1, 2020 · 7 comments
Closed

Add MVar.modify? #771

scf37 opened this issue Feb 1, 2020 · 7 comments

Comments

@scf37
Copy link

scf37 commented Feb 1, 2020

Similar to Ref.modify, but using pessimistic lock.

It is very useful to guard shared state where state modification is effectful.

  /**
   * Atomically modify existing MVar value of type `S`, returning computed value of type `A`.
   *
   * @param v MVar to use
   * @param f MVar value modification function, returning new value and additional result
   * @tparam F effect to use
   * @tparam S MVar value type
   * @tparam A computed value type
   * @return computed value or error if `f` failed
   */
  private def modify[F[_], S, A](v: MVar[F, S])(f: S => F[(S, A)])(implicit B: Bracket[F, Throwable]): F[A] =
    v.take.flatMap { st =>
      B.guarantee(f(st).flatMap { case (st2, a) =>
        v.put(st2).map(_ => a)
      })(v.tryPut(st).void)
    }
@kailuowang
Copy link

You might want to submit it to the cats-effect instead

@scf37
Copy link
Author

scf37 commented Feb 1, 2020

Opps, wrong repo indeed

@scf37 scf37 closed this as completed Feb 1, 2020
@rossabaker rossabaker transferred this issue from typelevel/cats Feb 1, 2020
@rossabaker
Copy link
Member

Moved to the correct repo and reopened.

@rossabaker rossabaker reopened this Feb 1, 2020
@scf37
Copy link
Author

scf37 commented Feb 1, 2020

Really, I do not think it is good idea anymore. This is non-reentrant and therefore very error prone.

Correct reentrant implementation of RefM allowing S => F[S] updates is complicated and likely can not be done on MVar.

Feel free to close this.

@SystemFw
Copy link
Contributor

SystemFw commented Feb 1, 2020

as it stands, the implementation is not safe:
v.take.flatMap means you can be interrupted after take, which would leave the MVar in an inconsistent state. if you take.bracket, now take is not interruptible, which isn't good either.

Until we have something like what I proposed in #681
this is going to be hard. We do have a safe version in Semaphore#withPermit which you can use to guard a Ref

@SystemFw
Copy link
Contributor

SystemFw commented Feb 1, 2020

The other thing worth mentioning is that while "shared state with lock" is easy for simple cases (and you can use Semaphore + Ref to get there) for more complex logic using Ref + Deferred instead tends to be way more easily correct and behave better wrt interruption, if not as easy to get into at first.

@rossabaker
Copy link
Member

Closing for now. Good conversation for the archives in case we revive it after something like #681.

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

4 participants