Add mutex to logger to avoid race condition

[dyastremsky]

Our logger uses std::stringstream, which is not thread-safe. Adding a mutex should remove possible undefined behavior (e.g. overlapping logs and segfaults).

Per the C++ 17 standard, "Concurrent access to a stream object (30.8, 30.9), stream buffer object (30.6), or C Library stream (30.12) by multiple threads may result in a data race (6.8.2) unless otherwise specified (30.4). [ Note: Data races result in undefined behavior (6.8.2). — end note ] – [iostreams.threadsafety]." (Source: https://berthub.eu/articles/posts/iostreams-unexpected/; in addition, there is this 2020 copy of C++ standard under 29.2.3).

[kthui]

I think the LogMessage::LogMessage() is a constructor and the stream_ is local to the LogMessage class. I am wondering how stream_ can be accessed concurrently on the constructor?

[nv-kmcgill53]

Are you able to reproduce this issue reliably enough to create a test out of it? Also is this something you ran into while developing or is there an issue related to this fix?

[dyastremsky]

I think the LogMessage::LogMessage() is a constructor and the stream_ is local to the LogMessage class. I am wondering how stream_ can be accessed concurrently on the constructor?

It looks like whenever a message is logged, a new log message is created. So my concern was that writing to the stream would not be thread-safe, even with different streams. It's hard to find information on this online, so perhaps that's incorrect? That's why I pinged the team, as if this eliminates race conditions, this would be a great change to get in, but I want to make sure I'm not missing anything.

[dyastremsky]

Are you able to reproduce this issue reliably enough to create a test out of it? Also is this something you ran into while developing or is there an issue related to this fix?

I can't reproduce the issue reliably. Let me provide some more context via Slack.

[nv-kmcgill53]

It looks like whenever a message is logged, a new log message is created. So my concern was that writing to the stream would not be thread-safe, even with different streams

Why wouldn't this be ok? If two different LogMessage()s get called on two different threads then each one of the streams are on their own thread. What you are saying would be a problem if we had one stream_ which is referenced in both threads but since they are their own memory then this should be fine.

[dyastremsky]

It looks like whenever a message is logged, a new log message is created. So my concern was that writing to the stream would not be thread-safe, even with different streams

Why wouldn't this be ok? If two different LogMessage()s get called on two different threads then each one of the streams are on their own thread. What you are saying would be a problem if we had one stream_ which is referenced in both threads but since they are their own memory then this should be fine.

Thank you for the feedback. It sounds like streams aren't global objects that get modified for each initialized stream. Appreciate the feedback from you and Kris here! Will close this out.