Strange types in BatMutex

[co-dan]

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]

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?