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

Mutex API ergonomics #336

Open
yawaramin opened this issue Oct 1, 2022 · 13 comments · May be fixed by #716
Open

Mutex API ergonomics #336

yawaramin opened this issue Oct 1, 2022 · 13 comments · May be fixed by #716
Labels
enhancement New feature or request

Comments

@yawaramin
Copy link

Can we draw inspiration from Rust's Mutex to make Eio.Mutex a bit more ergonomic? E.g. usage adapted from https://github.com/ocaml-multicore/eio#the-rest-mutex-semaphore-and-condition:

open Eio

let of_path = Mutex.create

let save m data =
  Mutex.use_rw m ~protect:true @@ fun path ->
  Path.save path data ~create:(`Or_truncate 0o644)

let load m = Mutex.use_ro m Path.load

We would need signatures like this:

module Mutex : sig
  type _ t

  val create : 'a -> 'a t
  (** Create in an unlocked state holding the data. *)

  val use_rw : protect:bool -> 'a t -> ('a -> 'b) -> 'b
  (** Lock for read-write access. *)

  val use_ro : 'a t -> ('a -> 'b) -> 'b
  (** Lock for read access. *)
end

Having said that, I realize we don't have borrow/move checking in OCaml so we wouldn't get the same level of safety. But it should make it a bit more ergonomic as the key idea is that a mutex is always paired with its data.

@talex5
Copy link
Collaborator

talex5 commented Oct 5, 2022

Yeah, I was wondering about doing that. In the end, I decided to make it look more like the stdlib's Mutex. I don't have enough experience with Rust to know if it's more ergonomic (I assumed Rust did it that way because it had no choice - it can't prevent concurrent access to the resource if it's outside the mutex).

Having the resource inside the mutex makes a lot of sense though, and would probably work better with OCaml's proposed modal types (haven't got access to the talk yet, so not sure).

But should use_rw also update the resource? e.g.

  val use_rw : protect:bool -> 'a t -> ('a -> 'a * 'b) -> 'b

@yawaramin
Copy link
Author

Interesting question, I think in my above example it is intrinsically 'updating' the resource (i.e. saving to the path) but the update is not reflected in the type. The 'b return type of the callback is just allowing us to return whatever we want from the operation. I figure if we are mutating the resource in the callback (say e.g. a cache), there's no need to return an updated value, the original value is mutated by the time the callback returns and the next use_ro or use_rw will see the mutated value.

@talex5
Copy link
Collaborator

talex5 commented Oct 6, 2022

Yes, it's probably not necessary to allow updating the resource itself. It would only be useful for something like an int, and you'd be better off with an atomic there. Also, people can always pass () as the resource and just use the API as before, if they want.

Are you planning to make a PR for this?

@yawaramin
Copy link
Author

Exactly. Sure, I'll take a crack at it!

@c-cube
Copy link
Contributor

c-cube commented Oct 6, 2022

I have this in containers. Note that update_map is similar to @talex5 's use_rw above. I think it's useful to be able to update the state if one wants (and also to just return a value, or just update (see CCLock.with_lock and CCLock.update).

Updating the value is useful if the lock protects a purely functional value, e.g. a Map.S.t or some record of functional values. Returning a side value is also useful; I don't have a public example ready but basically it can be nice for this kind of thing:

let update_my_resource (r: _ CCLock.t) =
  let action =
    (* critical section here *)
    CCLock.update_map r (fun cur ->
      let new_value, action = (* something using [cur] *) in
      new_value, action)
  in
  (* perform action outside the critical section *)
  perform action

Note that we both want to update the lock's protected content, and to return something to do later (or, say, a flag indicating what to do next).

@yawaramin
Copy link
Author

I have to admit, I don't quite understand why we need a mutex to protect the update of a purely functional immutable value like say a Map.S.t or an int. 'Updating' such values doesn't actually 'change' the initial value, it just creates a new value. Observers (i.e. readers/writers) of the initial value would not experience any data races, I think.

If we are making the argument that we need to update an immutable value and synchronize that update among multiple observers, then we would probably want to use an Atomic reference to safely do the update, no?

@c-cube
Copy link
Contributor

c-cube commented Oct 9, 2022

You can do the same with an atomic if the critical section is idempotent (since you might have to do it several times under contention) and if it's cheap (ditto). A mutex is more general and sometimes more convenient. If you just update an int, of course, it might be that Atomic is enough.

@talex5
Copy link
Collaborator

talex5 commented Oct 9, 2022

In @c-cube's API, with_lock and update_map are separate functions, which seems sensible. If Eio does the same, then we can start an immutable resource field, as @yawaramin proposes, and then make it mutable in a later PR if needed, without breaking existing code.

@c-cube
Copy link
Contributor

c-cube commented Oct 11, 2022 via email

@yawaramin
Copy link
Author

Sure. Let's say you have a complex data type cache = string Map.Make(String).t (for the sake of argument). So wouldn't you use cache ref Mutex.t to synchronize it safely across multiple fibres? I don't see how you could use cache Mutex.t. The cache is immutable, if you calculated a new value while holding the lock, how would you 'save' the new value? Are you saying that the mutex itself should store a reference to the value i.e. the mutex would be a fiber-safe ref?

@c-cube
Copy link
Contributor

c-cube commented Oct 12, 2022

The later, yes. In containers:

(* in CCLock.ml *)
type 'a t = { mutex: Mutex.t; mutable content: 'a }

@yawaramin
Copy link
Author

Wouldn't this be a duplication of mutability if we are already dealing with a mutable data structure e.g. let's say now type cache = (string, string) Hashtbl.t. Then should we be mutating the hash table value in place while holding the mutex locked, or immutably updating it and then setting the new hash table as the content of the mutex, or both? The state space has expanded.

In my view it would be simpler to separate the concepts of 'holding the lock' and 'mutating the data'. If you have an immutable data structure, then you could just use a e.g. int ref Mutex.t (or a new record type with a mutable field or even an array if you prefer). If you have a mutable data structure, then you use e.g. (string, string) Hashtbl.t Mutex.t directly.

@c-cube
Copy link
Contributor

c-cube commented Oct 12, 2022

It's this way because it's more flexible. No one forces you to actually mutate it, and some functions assume you use internal mutation (like with_lock). Why introduce a wasteful indirection with a ref?

I know that if I want to protect data, I just use foo CCLock.t, whether foo is mutable or not. It's pretty straightforward this way and I won't have to change it.

@talex5 talex5 added the enhancement New feature or request label Feb 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants