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

Strange types in BatMutex #881

Open
co-dan opened this issue Nov 9, 2018 · 1 comment
Open

Strange types in BatMutex #881

co-dan opened this issue Nov 9, 2018 · 1 comment

Comments

@co-dan
Copy link

co-dan commented Nov 9, 2018

I am not really sure if this is a bug or a misunderstanding on my part.
I was looking into the BatMutex and BatConcurrent modules, and noticed the following.

BatMutex provides two functions:

val make : unit -> BatConcurrent.lock
val synchronize : ?lock:Mutex.t -> ('a -> 'b) -> 'a -> 'b

Because make returns BatConcurrent.lock, you can't really use it with synchronize, i.e the following code does not typecheck:

let () =
  let m = BatMutex.make () in
  BatMutex.synchronize ~lock:m (fun x -> x) ()

Perhaps I am just not experienced enough with Batteries, but I don't understand the point of the BatMutex API.
The function synchronized cannot be used with the only other function in that module.
What is more, BatConcurrent also has a function called synchronize, but it really has a different type.
BatMutex.synchronize is more like BatConcurrent.sync if I understand it correctly.
So, the following code does typecheck:

let () =
  let m = BatMutex.make () in
  BatConcurrent.sync m (fun x -> x) ()

It it a "bug" in the type of BatMutex.synchronize or am I just misunderstanding the library?

@rixed
Copy link
Contributor

rixed commented Nov 22, 2018

My understanding is that synchronized and make are totally unrelated.

BatMutex.make is used to create a BatConcurrent.lock, which as you can see here (https://github.com/ocaml-batteries-team/batteries-included/blob/master/src/batConcurrent.mli) is an abstract lock that can be either a Mutex, a RMutex or... nolock.
This abstract lock is used here and there in batteries itself to optionally protects the bits of code that is not reentrant, so that the user can choose to use a proper lock (say, one based on a proper mutex and obtained with BatMutex.make) by overriding the internal lock. See for instance in batUnix.ml (https://github.com/ocaml-batteries-team/batteries-included/blob/master/src/batUnix.mlv#L83).

You could argue that there should be a make_mutex and make_rmutex in BatConcurrent but I guess that was done this way around to avoid depending on -thread in the general case which seems to be nolock.

In many places though, including in batPervasives, the mechanism is advertised but actually not used.
This whole BatConcurrent looks unfinished, neglected, and a bad idea to begin with. If batteries internals rely on some mechanism that are non-reentrant, then they should be made safe by default and always and not require the user to first override a global ref to some nolock (that is, if he remembers about it and have time to do so in the module initialization code, and without overridding other choices made by other libraries also depending on batteries). If we want to support several mutex libraries then those modules could rely on functors and/or provide variants of those modules?

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

2 participants