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

AtomicCell#get should not semantically block #3518

Merged

Conversation

armanbilge
Copy link
Member

@armanbilge armanbilge commented Apr 1, 2023

At least, I don't see why it should. It seems to me that the primary concern is that get shouldn't see inconsistent state, otherwise it doesn't matter if someone happens to be modifying/updating the cell at that moment. Also there's no reason that concurrent gets should have to take turns acquiring the mutex; they can all just read.

cc @BalmungSan

@BalmungSan
Copy link
Contributor

Conceptually I agree, however I still think it would be best to have both options (and I personally think get should be the one blocking)

My rationale is that the point of AtomicCell is evalModify which means an update can be potentially long running enough to notice a happens before semantics between an update and a get, and thus I personally would expect the get to block and get the updated value.
Yet, I also see value in having an option to just read the current value without blocking.

@armanbilge
Copy link
Member Author

I think one way to frame the semantics question is: what happens to the value of the cell during a modification? Does it retain its current value until updated, or is it in some "indeterminate value" state?

If we say its "indeterminate" during modification, then I guess we can keep get as-is and introduce a tryGet: F[Option[A]] that doesn't block. But there would still be a fundamental race condition.

@BalmungSan
Copy link
Contributor

BalmungSan commented Apr 2, 2023

Well, another way to think about it, is that since an AtomicCell uses a Mutex, its internal value is a "exclusive shared state". Only one fiber at a time should have access to it.

The question then is, something like evalModify means I have access during the whole duration of the provided F, or just during the read and the final write?

@armanbilge
Copy link
Member Author

Writing is certainly exclusive, but not sure why reading has to be exclusive.

@durban
Copy link
Contributor

durban commented Apr 3, 2023

On the one hand, the get locking is already not very useful: e.g.:

c.get.flatMap { v =>
  /* here I don't have exclusive access, because the lock have been already released */
}

So get can only provide a "snapshot". So, not locking seems fine.

On the other hand, this:

I think one way to frame the semantics question is: what happens to the value of the cell during a modification? Does it retain its current value until updated, or is it in some "indeterminate value" state?

This seems like the answer would be application-specific. I was trying to come up with some scenarios for using AtomicCell, and thinking through this... and I couldn't. What is AtomicCell actually used for? (This might be a sidetrack, feel free to ignore me.)

@armanbilge
Copy link
Member Author

What is AtomicCell actually used for?

When you need to atomically update some state with the result of an effect that can/should not be run more than once.

If you just need to atomically update state with the result of a pure function, use Ref#update.

If you need to atomically update state with the result of an effect that is okay to run more than once, use Ref#access and retry as necessary.

@durban
Copy link
Contributor

durban commented Apr 4, 2023

When you need to atomically update some state with the result of an effect that can/should not be run more than once.

Sure. I just had trouble coming up with a scenario when that happens (e.g., the ParkingLot example in the scaladoc uses random number generation, which I'm pretty sure is a "rerunnable" effect). Thinking about it some more, I found some examples, but realized I wouldn't actually use AtomicCell (nor Mutex) to implement them. But I guess that's normal... e.g., if we squint hard enough, I'm pretty sure there is an (inlined and specialized) "mutex" (and thus, an "AtomicCell") in memoize somewhere.

Copy link
Member

@djspiewak djspiewak left a comment

Choose a reason for hiding this comment

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

I find @durban's argument to be pretty compelling here: get locking isn't really helpful.

@djspiewak djspiewak merged commit d334a81 into typelevel:series/3.x Apr 18, 2023
@BalmungSan
Copy link
Contributor

@armanbilge @djspiewak should we change evalModify to also only block during the write instead of the whole time then?

I would think that is a bad idea, but still don't understand why that read should block whereas this read shouldn't.

@armanbilge
Copy link
Member Author

armanbilge commented Apr 18, 2023

evalModify is different, because it is atomically modifying the current value. So it needs to be sure that nobody is changing the current value in the mean time, because then the modification would be invalid. So, it must acquire the lock before reading it.

@BalmungSan
Copy link
Contributor

@armanbilge right, I know that, I was just still confused why that "read" is different from this "read".
But, after a long night of debating with myself, I realized the key point.

get always gives you "stale" data, so if there is a concurrent long-running evalModify waiting for it to complete wouldn't give me a morally better value than just reading the current one. Since, even if we wait, by the time I flatMap and use that value, it may have already changed again.

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.

4 participants