Thread safety changes #19
Conversation
…omplain about missing implementations, but instead assume they will be in some .o file that is later passed to the linker.
…evel from stdlog's log level.
…oreach(). Possibly a compiler bug in 2.066 master.)
* User code can no longer directly read out the stdlog, because by the time it has the reference, it might already be obsolete. (Later an `opApply` style function could be provided to execute functions/delegates in the context of a locked stdlog.) * The stdlog property was split up into a public setter `stdlog` and a package getter `stdlogImpl`, because the compiler doesn't like different visibility for getter and setter. * All module level functions dealing with the stdlog now synchronize with the global `__stdloggermutex`.
…s generated and emitted for `isLoggingActive` and it can be called at runtime. This commit turns the template function into just a template `isLoggingActiveAt` and a global bool flag `isLoggingActive`, so they can both be evaluated without parenthesis now and don't end up as functions in the final executable.
…he logger package.
…able to gain more control over the implementation details when I implement thread safety measures. Made `fatalHandler` accessible as a property to allow synchronization in later commits. Also made `writeLogMsg` protected, so it can assume it is properly synchronized already by the public calls calling it. To log a pre-built message `forwardMsg` can now be used. (See `FileLogger`).
… would revert the setting. And while I was at it, I changed `stdlog`s behavoir on assignment of `null`: It will now put the default logger back in place.
Logger now has a mutex to protect its public API. The module level functions don't perform any runtime checks themselves to avoid races. Instead the logger does all checking under protection of its mutex. `globalLogLevel` will be evaluated only once for any public logging call and thus needs no global locking. All overrides of `writeLogMsg`, `beginLogMsg`, `logMsgPart` and `finishLogMsg` have `protected` visibility now, since they expect their logger to be already under mutex protection and must not be publically visible.
|
overall the patches appear to be very good. I will a closer look tomorrow or monday. You should create this PR for my phobos logger branch so your contribution shows up in git correctly. moving git history from one repo to the next is just a pain. anyway thanks alot |
| { | ||
| return this.logLevel_; | ||
| synchronized (mutex) return this.logLevel_; |
There was a problem hiding this comment.
Couldn't this use an atomic read instead? Locking seems overkill
How is that a problem? |
|
@JakobOvrum You are right, a single read doesn't require locking. I will change that. |
|
Being the |
|
Somehow I don't see the point in arguing that. I want to see questions on d.learn, StackOverflow or bug reports about not being able to read |
|
I don't see the point in |
|
Yes, we do expect properties to stay the same while we operate on them. That is because we work on them in a thread local contexts all the time. |
|
Ok, I reopened this pull at burner/phobos#2 as requested by Robert. |
This pull request addresses the mentioned basic thread safety issues while keeping the API mostly the same. In particular I changed the following:
writeLogMsg,beginLogMsg,logMsgPartandfinishLogMsg) are nowprotected. They are the ones who no longer need to implement thread-safety themselves and never get called by user code. They are only called by library code that synchronizes with the Logger's mutex before calling them. When implementing your own loggers, remember to not accidentally make the overrides public.stdlogcan no longer be read out since we cannot guarantee the returned pointer will still be thestdlogwhen we call a method on it. (For user code that needs to work withstdlogdirectly we could add a sort ofstdlogApplyfunction that takes a delegate with the code to execute in the context ofstdlog.stdlogApplywould lock the global mutex before executing said delegate.)stdlogwhen set tonullwill swap the default logger back in.randomString()no longer leak into user code.isLoggingActiveis now split into an compile time constant and a template (isLoggingActiveAt) for a specific level. The return values are the same, but code & symbols for the functions are no longer emitted.A word on Logger chaining: A special method for logging chains has been added to replace the now inaccessible
writeLogMsg()with the nameforwardMsg(). As public a method it will do the proper synchronization before callingwriteLogMsg()internally and not check theglobalLogLevelagain.Note: This pull request does not address the recursive logger call issue.