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 atomic modify+get to MonadState #120

Closed
wants to merge 9 commits into from

Conversation

kubukoz
Copy link
Member

@kubukoz kubukoz commented Jun 17, 2019

It has been mentioned on Gitter that such a function would be useful, especially for things like Ref that provide an atomic modify operation that allows returning a value depending on the previous state. Here it is.

It probably needs some laws, so I'll think about it and report back when I come up with something reasonable (will check what it looks like in Haskell world too).

@codecov-io
Copy link

codecov-io commented Jun 17, 2019

Codecov Report

Merging #120 into master will increase coverage by 0.34%.
The diff coverage is 94.44%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #120      +/-   ##
==========================================
+ Coverage   89.82%   90.16%   +0.34%     
==========================================
  Files          66       66              
  Lines         580      590      +10     
  Branches        2        2              
==========================================
+ Hits          521      532      +11     
+ Misses         59       58       -1
Impacted Files Coverage Δ
.../src/main/scala/cats/mtl/laws/MonadStateLaws.scala 100% <100%> (ø) ⬆️
...ala/cats/mtl/laws/discipline/MonadStateTests.scala 100% <100%> (ø) ⬆️
core/src/main/scala/cats/mtl/instances/state.scala 95.45% <100%> (+11.24%) ⬆️
core/src/main/scala/cats/mtl/MonadState.scala 90.9% <80%> (-9.1%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2451abf...e9b9478. Read the comment docs.

@kubukoz
Copy link
Member Author

kubukoz commented Jul 30, 2019

@kailuowang could you take a look? Do you think we should add any laws for this?

@kailuowang
Copy link
Contributor

I am on vacation at the time. Will take a proper look once I am back to my computer. In the mean time please feel free to ping other maintainers if you want quick feedback.

@kubukoz
Copy link
Member Author

kubukoz commented Jul 30, 2019

Sure, enjoy your vacation :)

@LukaJCB if you have a moment...

@LukaJCB
Copy link
Member

LukaJCB commented Jul 30, 2019

Looks good to me thus far, though I wonder what practical implications this has. I don't think we can actually guarantee that these are implemented atomically, can we?

@kubukoz
Copy link
Member Author

kubukoz commented Jul 30, 2019

@LukaJCB it basically boils down to the fact that the current methods on MonadState don't include one that'd correspond to Ref's modify (modify on MonadState is update on Ref), so using e.g. meow-mtl to derive a scoped instance of MonadState (as in, an instance that'll update part of the state) makes it impossible to keep an atomic update+get operation in the form of Ref's modify.
We can't guarantee atomicity, but I think it'd still make the situation better if we had this. We can also add laws that'll check basically what the default implementations does (e.g. inspect can be done with state), at least.

@LukaJCB
Copy link
Member

LukaJCB commented Jul 30, 2019

Sounds good to me, let's add those consistency laws then and we should be good to go :)

@kubukoz
Copy link
Member Author

kubukoz commented Jul 31, 2019

@LukaJCB I added these laws, and as I understand they should be internal. Do they make sense?

@kubukoz kubukoz changed the title WIP: Add atomic modify+get to MonadState Add atomic modify+get to MonadState Jul 31, 2019
@SystemFw
Copy link
Contributor

SystemFw commented Jul 31, 2019

The issue here is that atomic modify is not about adding laws, but relaxing them.
For example, the key law in MonadState is:

set(a) >> get = set(a) >> a.pure

but you cannot rely on this law in substitutions where concurrency is involved:

(set(a) >> get, set(b).parMapN((a,_) => a) // is not the same as
(set(a) >> a.pure, set(b)).parMapN((a, _) => a)

The latter always returns a, the former returns a or b depending on which interleaving you pick (e.g. set(a), set(b), get)

@SystemFw
Copy link
Contributor

def stateIsGetAndModify[A](f: S => (S, A)) = {
   state(f) <-> (get.map(old => f(old)._2) <* modify(old => f(old)._1))

This one seems also incorrect to me, there could be a concurrent update in between get and modify

@kubukoz
Copy link
Member Author

kubukoz commented Jul 31, 2019

It's true @SystemFw, I was going to mention it at some point but forgot...

I added this stateIsGetAndModify law only because set(a) >> get = set(a) >> a.pure was already there - in my opinion we should have neither, but that's relaxing the laws a lot... think I should get rid of them?

@SystemFw
Copy link
Contributor

think I should get rid of them?

I think that's the crux of the discussion: is MonadState an abstraction over sequential state like State, or does it encompass concurrent state as well?

@kubukoz
Copy link
Member Author

kubukoz commented Jul 31, 2019

paging @oleg-py as meow-mtl's usecase is one of the reason this PR exists

@kubukoz
Copy link
Member Author

kubukoz commented Jul 31, 2019

is MonadState an abstraction over sequential state like State, or does it encompass concurrent state as well?

I'd say yes, and I'm all for relaxing the definition to just a few state actions that are atomic by themselves.

@SystemFw
Copy link
Contributor

I'd say yes, and I'm all for relaxing the definition to just a few state actions that are atomic by themselves

How is the resulting thing different from Ref though? (real question)

@kubukoz
Copy link
Member Author

kubukoz commented Jul 31, 2019

Classy lenses. Can you create a Ref[B] from Ref[A] given a Lens[A, B]?

@kubukoz
Copy link
Member Author

kubukoz commented Aug 7, 2019

@SystemFw could you take a look? Ping me on gitter if you think we need a longer discussion :)

@neko-kai
Copy link
Contributor

neko-kai commented Aug 8, 2019

@kubukoz

Classy lenses. Can you create a Ref[B] from Ref[A] given a Lens[A, B]?

Unrelated, but with a ZRef you can :^)

@kubukoz
Copy link
Member Author

kubukoz commented Sep 8, 2019

@neko-kai

@kubukoz

Classy lenses. Can you create a Ref[B] from Ref[A] given a Lens[A, B]?

Unrelated, but with a ZRef you can :^)

Is it even merged? ;)

@SystemFw
Copy link
Contributor

SystemFw commented Sep 8, 2019

Classy lenses. Can you create a Ref[B] from Ref[A] given a Lens[A, B]?

I meant conceptually. The same code that is used in meow-mtl to create a MonadState can be used to create a Ref. If you add this modify to MonadState it seems to be that it's just Ref up to renaming (and lacking a default Sync[F] => MonadState[F, A] through AtomicReference). Still, we can do this, I'm not opposed

@kubukoz
Copy link
Member Author

kubukoz commented Sep 8, 2019

Conceptually it's the same thing as what meow-mtl does for MonadState in general. The only other thing that this adds beyond what a custom function creating a "scoped" Ref would be an abstraction over that and State.apply.

@kubukoz
Copy link
Member Author

kubukoz commented Sep 8, 2019

Maybe we should consider having different abstractions for (potentially) concurrent and sequential state. The latter would have more laws, and would be able to get a default implementation of this method by using get + set. Something like MonadState <: AtomicState... With better names.

@kubukoz
Copy link
Member Author

kubukoz commented Feb 8, 2020

Alright, here it goes:

What we agree on:

  • equivalencies like set(x) *> get <-> set(x) *> x.pure[F] aren't true in the presence of concurrent access

I would also state that:

  • def state[A](f: S => (S, A)): F[A] has a place in MonadState
  • all the other methods in MonadState could be derived from def state

What I suggest:

  1. Add def state to MonadState
  2. Weaken the laws so that they are true in the presence of concurrency - that would probably only leave laws about methods being possible to derive from state, e.g. state(_ => (s, ())) <-> set(s)
  3. Add trait SequentialState[F[_], S] extends MonadState[F, S] with laws that can only be true for implementations that don't allow concurrency, i.e. StateT - that would include existing laws removed in step 2
  4. (Optional) Add trait ConcurrentState[F[_], S] extends MonadState[F, S] with laws that take concurrency into account - that would require a dependency on cats-effect, and checks like "if these two write concurrently, both actions will be atomic". I don't think this one will bring much value though.

Alternatively, we could stop after step 2 and only weaken the laws + add state.

cc @SystemFw @benhutchinson

@benhutchison
Copy link
Member

Im +1 for steps 1,2,3. I have an existing use case for SequentialState so please include it.

I think SequentialState could be implicitly viewable as ApplicativeAsk without the problems Fabio described at #179 (comment) , namely that doubly-asking might yield different results

Apologies for the slow reply (the @mention had my name slightly misspelled so it didnt fire)

@kubukoz
Copy link
Member Author

kubukoz commented Mar 6, 2020

My apologies for the misspelling @benhutchison, it's not even the first time...

I'm not entirely sure SequentialState implies immutable Ask - even in State, doing State.get (which is pretty much ask) can yield different results if you put writes between such calls.

@benhutchison
Copy link
Member

IMO what's important is that this law is satisfied, which an ask based on SequentialState would:

(ask, ask).tupled <-> ask.map(x => (x,x))

@rossabaker rossabaker changed the base branch from master to main August 19, 2021 13:10
@kubukoz kubukoz closed this Oct 5, 2022
@kubukoz kubukoz deleted the state-state branch October 5, 2022 22:24
@benhutchison
Copy link
Member

@kubukoz can you comment on why this is closed? I still feel there is a real issue here, and discussion in this thread improved my thinking about State.

@kubukoz
Copy link
Member Author

kubukoz commented Oct 5, 2022

@benhutchison the thread has been inactive for quite a while, I personally have no interest in revisiting this until I have another usecase, and finally - the whole library had a major refactor/rewrite so names like MonadState don't apply anymore. It'd have to be reimplemented from scratch.

And I was just cleaning my old PRs :)

@benhutchison
Copy link
Member

The aspect of this thread I found thought-provoking & valuable was the idea that State might come in two flavors: a weaker concurrent state where the state is unstable/variable between writes to the state (eg a concurrently modified Ref), and a stronger traditional State where one can read back whatever was last set into the state store.

I hadn't encountered that distinction before in other libraries. It feels "real" but I'm not sure if it's practically useful.

It suggests there's a similar distinction in Reader, between reading from an unstable environment and reading from an immutable one.

@benhutchison
Copy link
Member

It's hard work keeping track of everything that moves on the plain

I just noticed Arman's ConcurrentStateful

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants