Closed Julow closed 5 years ago
A quick look looks good for me but here a few questions for you (bear with me I don't have the code in my head):
Did you investigate if there's maybe not a more outoumatic way of doing this ? I think it's too much work to have to wrap your reporters. More specifically I'm thinking about a function in Logs
to set a mutex : (unit -> unit) -> unit
function to be used with the reporter invocation. The bare logs library should have this function as a nop, linking the threads
library simply sets that mutex
function with an appropriate implementation for the threads
library as a toplevel side effect. It's also a more general mecanism that can be used with possibly other forms of concurrency. Or is that too much magic and/or unworkable ?
I'm always confused about thread
vs threads
(e.g. in ocamlbuild
) and @yallop's ctypes uses threaded
. Maybe Logs_threaded
would be a better a name ?
threads
is the library name (that's why I choose Logs_threads
) and thread
is the command line flag for ocamlopt
.
Maybe threaded
will let users think the library itself uses threads to print logs in parallel to the application ?2. Maybe
threaded
will let users think the library itself uses threads to print logs in parallel to the application ?
I think the idea is rather that you should use the library in a threaded
program. In any case it would be nice not to multiply the conventions. An ocamlfind list | grep thread
doesn't give much here except for ctypes, maybe you have more and following the convention you propose. If not, I'm willing to follow @yallop's convention here.
I could find other examples: oUnit.threads
and containers.thread
. I think there is no convention on this.
I changed to threaded
.
I experimented a bit with a more general/magical way of solving this but failed. My main problem is that the initialization code is not called if the module is not used. Also, because of type escaping their scope, the implementation is not as safe as it could. (https://github.com/dbuenzli/logs/compare/master...Julow:thread_safe_general)
I prefer the approach of this PR. I think the more declarative and explicit code will be easier for users. They can't forgot to link the library and a reporter must be set to show the logs. Wrapping reporters is easy and can be done outside of the library.
My main problem is that the initialization code is not called if the module is not used.
These kinds of problems usually call for -linkall
. But I don't mind having an API entry point (e.g. Log_threaded.enable ()
) that users need to explicitly call, it's also better for automatic dependency discovery.
Also, because of type escaping their scope, the implementation is not as safe as it could.
I don't understand this comment.
I think the more declarative and explicit code will be easier for users.
The only thing I fear a bit is that people will start wrapping their reporters thinking "I want my reporter to by thread safe" whereas only the bit that sets the reporter should be doing it. So I think I'd still favour the approach above.
Hi !
With @TheLortex, we needed log in a threaded program.
This PR adds the
Logs_threads
library (logs.threads
package) and theLogs_threads.thread_safe_reporter
function that make any reporter thread safe.Without this, the output of
test/test_threads.ml
looks like this: