Open skaravos opened 2 years ago
First of all, there is a point of confusion here that need clearing up: [s_stderr_indentation](https://github.com/emilk/loguru/blob/89f06b3919413b978a65eac71885f5c0d71b50c2/loguru.cpp#L204)
is thread-safe, because it uses atomics. It is thus intentionally, and safely, shared between threads.
Whether or not this is good behavior depends on context.
When I wrote it I had this case in mind:
void do_jobs() {
LOG_SCOPE_F(INFO, "doing_stuff");
thread t1([](){
// logs a few lines (e.g. warnings) but doesn't create scopes
});
thread t2([](){
// logs a few lines (e.g. warnings) but doesn't create scopes
});
t1.join();
t2.join();
}
in this example the current behavior is better, as the log lines in the child threads are correctly indented.
So just switching to thread local indentation level will make some use cases worse.
We could consider having a compile-time switch for it.
Another solution is to have a mechanism to copy (or not) the current indentation level to a child thread. Perhaps even trace the parent thread with something like:
auto parent = loguru::thread_info();
thread t1([=](){
loguru::set_parent_thread(parent); // copy indentation level, and print parent thread name in error context
});
Agreed that it's 100% thread-safe from a data-access point of view, certainly nothing is corrupted and no messages are lost. Sorry for the confusion there.
I was referring to a stretched definition of "thread-safe" whereby the current implementation causes indentation behavior on any given run be non-deterministic. Specifically: the same logged message will be logged with a different indentation level depending on which threads finish first.
Seeing your original intent, I now understand why you chose to use a regular static variable.
However, I really think LOG_SCOPE_F
has the potential to be a powerful feature that is limited by essentially restricting its use to a single parent thread.
Both of your suggestions would solve the problem, although the second solution is more flexible. Passing some informational context from the parent thread could allow threads to inherit more than just indentation information.
In fact, that solution would result in the exact behavior I had originally expected after reading the docs.
Just as an example: This is what I thought would happen when I originally ran my test program. (The child threads maintain their own depth counter that was initialized based on parent depth)
...[main ] loguru.cpp:645 INFO| arguments: /real/path/removed/TestLoguru.exe
...[main ] loguru.cpp:648 INFO| Current dir: /real/path/removed
...[main ] loguru.cpp:650 INFO| stderr verbosity: 0
...[main ] loguru.cpp:651 INFO| -----------------------------------
...[main ] main.cpp:10 INFO| { main()
...[thread1] main.cpp:16 INFO| . { thread1()
...[thread1] main.cpp:17 INFO| . . sleeping 1 sec...
...[thread2] main.cpp:25 INFO| . { thread2()
...[main ] main.cpp:31 INFO| . joining...
...[thread2] main.cpp:26 INFO| . . sleeping 3 sec...
...[thread2] main.cpp:27 ERR| . . this is an ERROR
...[thread1] main.cpp:16 INFO| . } 1.029 s: thread1()
...[thread2] main.cpp:25 INFO| . } 3.070 s: thread2()
...[main ] main.cpp:10 INFO| } 4.126 s: main()
...[main ] loguru.cpp:524 INFO| atexit
With that said... the compile-time switch is a lot easier to implement and would avoid any runtime overhead, so maybe that's the better solution.
P.S. somewhat related: it would be nice to have a way to disable indentation or change the width without also disabling the preamble (maybe a LOGURU_SCOPE_INDENTATION_WIDTH
definition that defaults to 4?)
Sorry for resurrecting an old thread but it could be useful to add LOG_GSCOPE_F
(for global scope), LOG_TSCOPE_F
(for thread scope), both of which would in turn call the LOG_CSCOPE_F
(for custom scope) that takes an additional scope object as an argument. Adding a compilation flag would be cumbersome and wouldn't really solve the problem.
At this point maybe LOG_SCOPE_F
(which became just an alias to LOG_GSCOPE_F
) would have to be deprecated to avoid confusion.
First off, I just want to say that the indentation feature is a great idea!
However the current implementation of
LOG_SCOPE_F()
really doesn't support multi-threaded programs, in fact its more of a bug than a feature.The current indentation level of the log is maintained in a global static variable,
s_stderr_indentation
that is shared by all running threads. CallingLOG_SCOPE_F()
from multiple threads at once essentially wrecks the indentation of the entire log.Suggestion:
Mark the indentation variable as
thread_local
, so each running thread maintains its own indentation stack. (I realise that some platforms do not support thethread_local
keyword so that may pose a small challenge.)Example Code
Here's a very simple program you can compile to see what I'm referring too
If you don't want to compile it yourself, here's a sample of the output I get when running the executable compiled by the above code.
Original Output
When you pay close attention to the threads, you'll notice that messages print at the wrong indentation level!
... just imagine what this would look like if your program has 10+ concurrent threads ...
Output after marking
std_stderr_indentation
asthread_local
IMHO this is much more readable than the original especially once you start grep'ing for output on specific threads.