Rationale behind thread_local for color_mode singleton

[ken-matsui]

toml11/include/toml11/fwd/color_fwd.hpp

Line 47 in 5727819

static thread_local color_mode status;

IIUC, a singleton is typically meant to provide a single shared instance across the entire application regardless of the number of threads. Can I know your rationale for using thread_local for color_mode?

[ToruNiina]

According to N3337 §3.7.2 [basic.stc.thread],

All variables declared with the thread_local keyword have thread storage duration. The storage for these
entities shall last for the duration of the thread in which they are created. There is a distinct object or
reference per thread, and use of the declared name refers to the entity associated with the current thread.
A variable with thread storage duration shall be initialized before its first odr-use (3.2) and, if constructed,
shall be destroyed on thread exit.

It means that you will implicitly make different object per thread by adding thread_local to a variable. There will be the same number of objects as threads (that uses the variable), and pointers/references to the variable points to the object stored in the thread in which the reference is taken. Each thread (including the main thread) initializes the object independently when the control reaches to the line where the object is. And the object will be destructed when the corresponding thread exits.

So, roughly speaking, we will have different static variable per thread. This allows us to call enable/disable/should_color from threads in parallel. That also means that enable/disable will be effective only within the same thread and you need to call enable/disable as many times as you create a thread (if we need to change the state from default). It could be inconvenient in some cases. Is this what you are concerned?

[ken-matsui]

Yes, this is what I am concerned about. I was thinking there were quite few use cases where we would want to use different color modes per thread. What would you think?

[ToruNiina]

Well, there is not such a strong motive here, simply that I was concerned about not being thread-safe.

I was thinking that there can be an implementation like, one thread takes toml::value or its location and outputs info/warn messages to a log file(w/o color), and another thread loads the data, do some calculation, and outputs warn/errors to the console (w/ color). However, this scenario doesn't seem very common.

In my use cases, I usually read one or a few configuration files first in a single thread at the beginning, and then the parallelized code runs based on that input, so the implementation of this part is not really an issue. Since your code is likely to actually read files in multiple threads, if it is actually inconvenient there, I think that means we should consider changing it.

[ken-matsui]

You might be talking about a different location of thread-safety, but a singleton seems to be thread-safe.

https://stackoverflow.com/a/19907903

[ToruNiina]

initialization is ok, but I think changing state from threads is not. no?

[ken-matsui]

Hmm, that's true. Then, I think using std::mutex::lock makes more sense. WDYT?

[ToruNiina]

I thought about using mutex and atomic when I added thread_local, but at that time I didn't chose it.

Consider the case where threads with different color settings exist. In such a situation, it is almost meaningless to protect only state updates with the enable/disable functions. If another thread changes the state during the execution of format_error, the color settings will become inconsistent while the message creation.
The correct approach would be to add functions that take a lock to prevent other threads from changing the color settings. However, this would cause waiting between threads. If the settings are copied per thread, each thread can control its state and can run without waiting.

In summary, there are the following differences between these two options:

• thread_local
  • No need to take a lock, allowing each thread to execute independently
  • Color settings are not shared, requiring each thread to redo the settings (if it need to change the behavior from the default)
• static
  • Requires taking a lock while calling a function, potentially causing threads to wait others
  • Color settings are shared, so nothing needs to be done if the settings are the same

If threads with different color settings run simultaneously, thread_local would be preferable. Conversely, if all threads run with the same color settings, mutex would be preferable.
Since I do not know much about how users (except me) use it, I did not have the material to prioritize any specific situation. Therefore, I chose the option that would be safe without adding much to the implementation.

However, if you found this choice makes it inconvenient, we need to reconsider.

It is an old-fashioned solution, but what about adding a macro like TOML11_THREAD_LOCAL_COLORIZATION and adding thread_local to the state only when it is defined?
In most cases, it is expected static would be enough. So, by default, we can make it static and switch to thread_local only when configured.

Also, having a mutex internally seems to be a tough task. For example, let's say we added lock_guard to the format_error function from the beginning to the end. This would prevent the color from changing in the middle of the message. However, users are likely to call larger units such as toml::find or toml::parse. If color_mode is changed by another thread during their execution, the resulting message might not be colored as expected. So, basically we need recursive_mutex and need to add lock_guard every function that may call color-related functions. That means that, if a thread tries to change colorize state, it may need to wait the whole toml::parse. So, for that case, I think it is better to leave thread_local as an option.

[ken-matsui]

I agree with your idea of static by default and thread_local as a non-default option since TOML11_THREAD_LOCAL_COLORIZATION looks easy to implement and maintain while leavingmutex to the user's responsibility.