Bug 16232 – std.experimental.logger.core.sharedLog isn't thread-safe

std.experimental.logger.core.sharedLog effectively casts a Logger to/from shared. So you get a Logger that's shared among threads but it's not typed as shared. The predefined loggers may have all their operations properly synchronized, but a user-defined logger doesn't necessarily. ---- import core.thread: Thread; import core.time: seconds; import std.experimental.logger: Logger, LogLevel, sharedLog; class MyLogger : Logger { this() { super(LogLevel.info); } int x = 0; override void writeLogMsg(ref LogEntry payload) @safe { // exaggerated ++x int y = x; () @trusted { Thread.sleep(1.seconds); } (); x = y + 1; } } void main() { auto myLogger = new MyLogger; sharedLog = myLogger; auto t = new Thread({ sharedLog.log(""); }); t.start(); // exaggerated ++myLogger.x int y = myLogger.x; Thread.sleep(2.seconds); myLogger.x = y + 1; t.join(); assert(myLogger.x == 2); /* fails */ } ----

There is nothing we can do about user defined code

(In reply to Robert Schadek from comment #1) > There is nothing we can do about user defined code We have to accommodate for it. sharedLog suggests that it takes care of thread safety, but it doesn't. Either is has to actually make things safe, or it shouldn't suggest that it does. The latter would mean letting the user cast to/from shared.

I will add a comment to make that clear

Commit pushed to master at https://github.com/dlang/phobos https://github.com/dlang/phobos/commit/7d7ce4a5ebaca7155e754b1bf7b45366e87d0194 Logger sharedLog comment on thread-safety fix Issue 16232 - std.experimental.logger.core.sharedLog isn't thread-safe

Commit pushed to stable at https://github.com/dlang/phobos https://github.com/dlang/phobos/commit/7d7ce4a5ebaca7155e754b1bf7b45366e87d0194 Logger sharedLog comment on thread-safety

(In reply to Robert Schadek from comment #3) > I will add a comment to make that clear Reopening. I think a comment isn't enough. sharedLog is marked as @safe, but it effectively casts shared away, which isn't safe. It can lead to memory corruption when shared data can be accessed as unshared. I see two ways out: 1) Make sharedLog @system. 2) Return a shared Logger. Lengthy example of @safe violation with custom Logger: ---- import std.experimental.logger.core: Logger, LogLevel, sharedLog; class MyLogger : Logger { ulong* p; this() @safe { super(LogLevel.all); /* Allocate a ulong that disrespects cache line boundaries (64 bytes), so that it won't be loaded/stored atomically. */ align(64) static struct S { align(1): ubyte[60] off; ulong x = 0; } auto s = new S; this.p = &s.x; assert((cast(size_t) p) % 64 == 60); assert((cast(size_t) p) % s.x.sizeof == 4); } override void writeLogMsg(ref LogEntry payload) @safe { assert(false); } /* never called */ } MyLogger sharedMyLogger() @safe { Logger logger = sharedLog(); return cast(MyLogger) logger; /* This is a simple downcast. Not casting away shared. */ } enum n = 1_000_000; /* Toggle *p between 0 and ulong.max (n times). */ void write(ulong* p) @safe { foreach (i; 0 .. n) *p = ~*p; /* non-atomic load and store */ } /* Assert that *p is either 0 or ulong.max (n times). */ void read(ulong* p) @safe { import std.conv: to; foreach (i; 0 .. n) { ulong val = *p; /* non-atomic load */ assert(val == 0 || val == ulong.max, val.to!string(16)); /* fails */ } } void main() { sharedLog = new MyLogger; /* Read and write concurrently. `read` will see a partially written value. I.e., memory corruption. */ import core.thread: Thread; new Thread(() @safe { write(sharedMyLogger.p); }).start(); read(sharedMyLogger.p); } ----

Commit pushed to dmd-cxx at https://github.com/dlang/phobos https://github.com/dlang/phobos/commit/7d7ce4a5ebaca7155e754b1bf7b45366e87d0194 Logger sharedLog comment on thread-safety

@burner created dlang/phobos pull request #8489 "sharedLog returning shared(Logger)" fixing this issue: - sharedLog returning shared(Logger) Fixes #16232 changelog https://github.com/dlang/phobos/pull/8489

dlang/phobos pull request #8489 "sharedLog returning shared(Logger)" was merged into master: - 63f49d6687be573b6e6c8a5f7401c5a82ea865b9 by Robert burner Schadek: sharedLog returning shared(Logger) Fixes #16232 changelog -preview=nosharedaccess code review working on the tests whitespace spaces behind casts https://github.com/dlang/phobos/pull/8489