C ++ 11 Threaded polymorphism with less granularity
I am writing a logger and want to make it thread safe. I did it by following these steps:
class Logger
{
public:
virtual ~Logger();
LogSeverity GetSeverity() const;
void SetSeverity(LogSeverity s);
protected:
std::mutex mutex;
private:
LogSeverity severity;
};
void Logger::SetSeverity(LogSeverity s)
{
std::lock_guard<std::mutex> lock(mutex);
severity = s;
}
LogSeverity Logger::GetSeverity() const
{
std::lock_guard<std::mutex> lock(mutex);
return severity;
}
void Logger::SetSeverity(LogSeverity s) const
{
std::lock_guard<std::mutex> lock(mutex);
severity = s;
}
// StreamLogger inherits from Logger
void StreamLogger::SetStream(ostream* s)
{
std::lock_guard<std::mutex> lock(mutex);
stream = s;
}
ostream* StreamLogger::GetStream() const
{
std::lock_guard<std::mutex> lock(mutex);
return stream;
}
However, sharing the class requires this extremely redundant locking.
Two options that I see:
1) Calling this public function will block the whole object using a mutex in the class
Logger l = new Logger();
std::lock_guard<std::mutex> lock(l->lock());
l->SetSeverity(LogDebug);
2) Blocking the flow around each variable in the class
template typename<T> struct synchronized
{
public:
synchronized=(const T &val);
// etc..
private:
std::mutex lock;
T v;
};
class Logger
{
private:
synchronized<LogSeverity> severity;
};
However, this solution is very resource intensive, blocking for each item.
Am I on the right track or is there something I am missing?
source to share
First of all, you need to carefully consider the possible use cases:
- Do you really need the log to be so customizable?
- what properties can be initialized at build time?
- does it make sense to change all of them?
I have a strange feeling that you think of your classes in a very small perspective: "well, this is a magazine, so I put all the useful useful features (I might be wrong). Classes should have complete but minimal interfaces that explicitly represent what a particular class responds to. Think about it.
As for your multi-threaded problem: I don't think general log is a good idea. Personally, I always prefer thread specific primitives in such cases (one log per thread). Why?
- if the log entry is being written to a memory section, you only need to lock the memory block, not the log itself
- If writing to a file is writing to a file, your task is even easier - remember that the OS controls access to files, so you don't need to worry about writing two logs to the same section of the same file (you must create your own logger to was, but it's really not that difficult)
- bonus: different streams can write to different outputs if needed
If your compiler supports C ++ 11, this solution is mostly correct usage thread_local
, __declspec(thread)
or __thread
whichever your compiler supports.
If you still want to implement a generic logger, start by looking at the project. For example: are you sure you need a mutex lock to change one property? Things like a member severity
are the perfect candidate for std::atomic
. This may take more work, but it can be much faster.
class Logger
{
//cut
private:
std::atomic<LogSeverity> severity;
};
void Logger::SetSeverity(LogSeverity s)
{
severity.store(s, std::memory_order_release);
}
LogSeverity Logger::GetSeverity() const
{
return severity.load(std::memory_order_acquire);
}
std::memory_order_acquire/release
- this is just an example - you can use a stronger ordering, for example memory_order_seq_cst
(if you need a full global ordering). However, a pair is acquire/release
usually enough to ensure proper synchronization between loads and stores and with a small bonus they won't create any fences on x86.
If you think you want to read C++ Concurrency in Action
Anthony Williams. It is the best resource for learning about threads, atomistics, synchronization, memory order, etc.
There are also some very good articles in the Bartosz Milewski blog . Like this one: C ++ atomicity and memory order .
If you are unfamiliar with topics like atomistics, fences, ordering, and more, these resources are very good to start with.
source to share
Suppose you need to access these setters and getters on different threads.
Maybe I'm wrong. But from the limited code you provided, the way you lock these members is simply wrong. It is not easy to block a paired setter and getter. Consider this:
void tYourClass::thread_1()
{
..
m_streamLogger.SetStream(/*new stream*/);
}
void tYourClass::thread_2()
{
ostream *stream = m_streamLogger.GetStream();
// access the returned stream
// stream->whatever()
}
In this case, right between you there is a thread handle and access to it, another thread starts and sets the thread. What will happen? You get a stream of sluggish : you can access a remote object or register something that will never be seen by others (depending on the logic inside SetStream
). Your castle just couldn't protect it. The main reason is that you must block statements that must be executed as a single "atomic" procedure, where no other thread can hit before it finishes.
And I have two suggestions.
- Don't put blocking inside setters / getters. This is either because it cannot protect everything as stated above, or because of its effectiveness. You can use these methods in one thread. In this case, you don't need a lock. Typically, setters and getters themselves cannot (and should not) understand how they will be accessed. So, a smarter place is to set a lock in the client code where you believe that multithreading is actually involved.
- Don't try to protect anything with one lock. It is suggested that the lock should be as short as possible. If you abuse one lock for a large number of independent resources, the degree of concurrency (one major thread's advantage) will be compromised.
source to share
First, I doubt if you really need these setters. The easiest way to make class flow safe is to make it immutable.
Even if you made them "safe", do you really want one thread to change the destination thread and the other thread to be in the middle of registration messages?
In this case, it looks like you could just set the severity and pairs in the constructor:
StreamLogger(LogSeverity severity, ostream& steam);
If the number of constructor arguments becomes unmanageable, you can create a builder or factory, or simply group your arguments into a single object:
StreamLogger(const StreamLoggerArgs& arguments);
Alternatively, you can highlight the parts of the logger that really need to be thread safe in the interface. For example:
class Logger {
protected:
~Logger(){};
public:
virtual void log(const char* message) = 0;
virtual LogSeverity GetSeverity() const = 0;
};
And it's an interface that you pass to multiple threads, a particular implementation can still have setters (not necessarily thread safe) if you want, but they are only used from one thread when the object is first set up.
source to share