Design review
I’m afraid this design is a bad idea.
I have seen multiple attempts to do something like this, and I understand the impulse. Synchronization is loud, dangerous, and takes up valuable code real estate in your algorithms, making it harder to suss out what’s actually going on. It’s only natural to want to hide all that behind a clean, simple interface.
But maybe it should be loud, dangerous, and obtrusively visible.
Maybe, as with other dangerous stuff, like type-punning, it should be clearly visible where you’re doing it in the code. Like reinterpret_cast, the sight of synchronization primitives and locking could serve as a red flag: “there be dragons here; check this code carefully, and do not fuck with it unless you are absolutely sure you know what you’re doing.”
See, I would say the real problem is that you have data that is both shared, and mutable. That is the thing you should be trying to redesign your code to fix. The presence of the synchronization primitives and locking is a symptom of that underlying problem… it’s not the problem in-and-of itself.
Okay, but let me stop speaking in abstract and show you exactly why this design is bad:
This class can break your entire program, if only two instances of it exist. Actually, it only takes one instance of this class, and any other lockable.
Here’s how.
Imagine you have a program that has exactly two instances of MemDB in it. We’ll be unimaginative and call ’em data_1 and data_2.
Let’s say you have a situation where you need to code some kind of operation involving both data_1 and data_2. Let’s say you have to remove some data from one, and put it in the other. And let’s say that transaction has to be atomic… meaning it’s either completely finished, or not started at all; there can never be a situation where it’s only half-done. For example, there is some kind of periodic auditing operation checking for fraud or cheating or whatever, and if it sees that any data is “missing”, it will panic.
No problem, though. This is trivial to code. Just lock both data sets, do the operation while holding both locks, then unlock ’em both. Simple. It’s just this:
writer_1 = data_1.writer();
writer_2 = data_2.writer();
auto stuff = writer_1->take_stuff();
writer_2->give_stuff(std::move(stuff));
// both (hidden!) locks automatically release at end of scope
Piece of cake.
Except…
Somewhere else in your program, a different, though very similar, transaction is happening. And, perchance, it happens to be coded like this:
writer_2 = data_2.writer();
writer_1 = data_1.writer();
// transaction happens here, locks auto-released at end of scope
And now, without warning or ceremony, you start discovering that your program crashes… sometimes. Not always! It’s the absolute worst kind of bug; the kind that only happens periodically, frustrating your attempts to drill down what’s causing it.
Well, obviously, there are only six lines of code shown here, so it has to be one of those. Won’t be that simple in a real program, of course.
To understand the bug, imagine we’re running that first code block in thread 1, and immediately after the first line, the OS scheduler puts the thread to sleep, and gives thread 2 its time slice. So, in thread 1, data_1 is locked, data_2 is not.
Now thread 2, running that second code block comes to life. It locks data_2, then tries to lock data_1… but, oh, it can’t. data_1 is already locked (by thread 1). No problem, this is a blocking wait, so we’ll just put thread 2 to sleep, and go back to thread 1.
Thread 1 wakes up, and tries to lock data_2… but, oh, it can’t. data_2 is already locked by thread 2.
Both threads are now blocked, with no hope of release. Deadlock.
And as bad as that is, the worst part by far, in my mind, is that when I look at both code blocks above, I see absolutely no indication that there is any locking going on there. I have absolutely no reason to suspect that a deadlock might occur in either block.
The actual deadlock problem is trivial to solve in C++17 and better, but even way back in C++11, it’s not that hard. Assuming all the data members of MemDB were public, you’d do something like this:
// thread 1 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
std::lock(data_1.mtx, data_2.mtx);
auto lock_1 = std::lock_guard<std::mutex>{data_1.mtx, std::adopt_lock};
auto lock_2 = std::lock_guard<std::mutex>{data_2.mtx, std::adopt_lock};
auto stuff = data_1.data->take_stuff();
data_2.data->give_stuff(std::move(stuff));
// both (NOT hidden!) locks automatically release at end of scope
// thread 2 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
std::lock(data_2.mtx, data_1.mtx);
auto lock_2 = std::lock_guard<std::mutex>{data_2.mtx, std::adopt_lock};
auto lock_1 = std::lock_guard<std::mutex>{data_1.mtx, std::adopt_lock};
// transaction happens here, locks auto-released at end of scope
// C++17 solution //////////////////////////////////////////////
// thread 1 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
auto lock = std::scoped_lock{data_1.mtx, data_2.mtx};
auto stuff = data_1.data->take_stuff();
data_2.data->give_stuff(std::move(stuff));
// lock automatically releases at end of scope
// thread 2 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
auto lock = std::scoped_lock{data_2.mtx, data_1.mtx};
// transaction happens here, lock auto-released at end of scope
But, as you can see, that pretty much invalidates the entire point of MemDB and Accessor.
As I said, I get the impulse to hide the synchronization primitives… but I think that’s misguided. In both versions above—C++11 and C++17—the locking is clearly visible, and act as red flags so I know there are dangers here. If I want do do anything else in either transaction operation that might require another lock, I have a flashing indicator that I can’t just randomly do that lock anywhere I want… which might just lead to another deadlock. Instead, I can see, very, very visibly, that I need to also lock that lock right up at the top with the other locks. That’s why hiding locks is such a terrible idea.
And, by the way, this is just the tip of a very large iceberg. Because there isn’t just the possibility of multiple locks, there may be multiple lock types. For example, I may only want to attempt to lock the data, and if that fails because it’s already locked, I may decide to do something else instead. This may be particularly useful for idle saving; I may want to only save opportunistically, when the data isn’t being written elsewhere, so it’s basically a free action… but if the data is locked, then I may want to move on to something else. And it’s not just try-locking; there are tons of other useful patterns, like shared locking. In fact, shared locking for shared, mutable data can be an enormous optimization. But it’s not possible if you hide away the synchronization primitives.
The real moral here is that if you need locks—especially multiple locks—maybe you need to rethink your larger program design. That is why I think it is a good thing to leave all that locking and such clearly visible and out in the open. If you are worried about people screwing around with that mutable, shared data without properly locking it… then I’d say the problem you have to fix is either the mutable, shared data… or the incompetents you’re letting in your code base. Putting a band-aid on the mutable, shared data to hide it from view is not really a wise fix.
Aside from all that, there really doesn’t seem to be a lot of thought put into the design. Like, it’s riddled from top to bottom with weirdness and problems, big and small.
Like, why is save() private? That makes no sense at all. With your public API, I can’t unconditionally save whenever I want. I can only call idle(), which may not save if the timing is wrong. So if I’ve just done something important or am about to do something dangerous, you don’t want me to be sure the data is actually saved first?
And why is the save interval only settable at construction, and never possible to query? I might want to bump up the save interval at a critical portion of the program, but the rest of the time keep it very low, for efficiency reasons.
And why is the destructor virtual? Nothing else is virtual, so it’s impossible to modify any behaviour.
As for an example of a big problem, consider this:
auto data = MemDB<whatever>{...};
auto reader = data.reader();
data.idle();
In other words: calling any two member functions of the class in the same scope is dangerous… sometimes. The culprit? Hidden locks. There doesn’t seem to be any way to make the class’s interface safe.
And so on and so forth. All in all, this just looks haphazardly slapped together with no real concern for making it actually usable in a real-world program. And, the fact that it just doesn’t work, which even the most basic testing would have revealed, testifies to how slapdash the whole thing is.
Code review
You are missing all the headers you need to make this work. You are also lacking any comments, documentation, usage examples, and—most importantly—tests. And, believe me, tests really would have helped you out here.
template <class DataType>
class MemDB {
std::mutex mtx;
Hard-coding the mutex type may make sense in C++11. But C++17 brought us the wonderful std::shared_mutex. For shared, mutable data that is rarely mutated but often read, a shared mutex can be a substantial performance improvement.
So it might make more sense to allow the mutex type to be parameterized.
DataType *data;
I’m not entirely sure why you chose to use a pointer here. It doesn’t really make much sense. Normally using a pointer in a case like this would mean either that the data is being managed elsewhere, or that you are opting to make the type cheaply-movable (at the expense of costing more to construct). But the second case can’t apply here, because having a mutex data member means the class is non-movable in any case.
So… is the intention that the data is externally managed? That would be something that the documentation would explain. None of the rest of the API helps; the constructor also takes a pointer, with no explanation whatsoever. Is it supposed to be an owning pointer? Does this class take over ownership? I mean, I guess not because the destructor doesn’t clean it up… but then again, the constructor doesn’t work either (more on that in a bit). It’s all a mystery.
This also causes weirdness and confusion with const. What does a const instance of MemDB mean? I mean, on the surface, it means nothing now, because you can’t do anything with it (because none of the operations are const). But normally const means “read-only”. So… shouldn’t a const MemDB allow reading, but not writing? Why not?
std::atomic_bool data_changed;
atomic<bool> is probably not the right type to use here, though your choice is limited prior to C++20. But in any case, you could get significant speed-ups on some hardware by using acquire-release semantics.
For example, in save(), instead of:
data_changed = false;
You’d do:
data_changed.store(false, std::memory_order_release);
And when reading, you’d do an acquire.
But as of C++20, rather than a bool, a better choice here would be an atomic flag. (The reason you need C++20 is that prior to C++20, there is no way to non-destructively test an atomic flag.)
time_t last_save_time, save_interval;
Using the C time functions here is wrong, for a very tricky and subtle reason. I mean, generally using C library stuff in C++ is wrong, for a number of reasons, usually boiling down to “the C library version is hot garbage, and that’s why we made a new version for C++”. But in this case, there are subtle issues that go beyond the usual.
Okay, first of all, let’s get all the usual “the C library is garbage” stuff out of the way. The C library does not define… anything… about time_t… other than that it’s an arithmetic type. You assume that it has a resolution granular enough for your save delay… but for all you know, the resolution might be days.
That also means that, for example, when you write something like time_t save_interval = 300, you’ve just written gibberish. You assume that means 300 seconds… but there’s no reason why it has to be. time_t could be a floating point type measured in years. That’s perfectly legal. Your default save interval may be in centuries.
You also seem to assume that time_t{0} is sometime “before” any other time. But… it doesn’t have to be. time_t{0} could very well be in the future. It could even be decades in the future.
Okay, okay, but maybe you’re assuming that POSIX standards apply, which require that time_t is an integer, the resolution is seconds, and the epoch is January whatever in the 70s. Everything solved, right?
Well… this is where the subtle part comes in. You see, you are using std::time(nullptr) to get the current time, and using that in arithmetic operations to compute durations. And that’s just fine… assuming the time doesn’t change.
You see, whatever clock std::time() is using, it isn’t guaranteed to be steady. That means that you might end up in a situation like this:
- Some program/daemon/whatever is running, and
save() is called, which gets the current time as 21 October, 2022, 7:28 AM.
- The user realizes, oops, I accidentally travelled in time to the 80s, so the current time is actually 26 October, 1985, 1:21 AM. They reset the computer’s clock to match. (I joke, but such time shifts could easily happen when testing. It could also happen in legit operation when a machine is moved across time zones. I mean… maybe not a 35-year time shift, but, yanno.)
- In that program/daemon/whatever,
idle() now gets called, and discovers that it will be almost 40 years before it needs to save the data again.
Now you’re counting on the data being periodically saved… but it ain’t.
What you should be doing here is using a steady clock. And there’s been one in C++ since C++11.
That will fix all of your problems.
- A steady clock will never regress. That’s the whole point of it. So you can always be 100% sure that if two calls to the clock say 10 seconds have passed, then 10 seconds have actually passed, and vice versa.
- You want to specify 300 seconds? Then specify it:
save_interval = 300s. Want 300 milliseconds? save_interval = 300ms. No mysteries here.
- You want to be sure that the resolution is at least a second? Well, you can’t control that… but you can check it at compile time, which is better than nothing.
Also, you get the benefit of type safety, because it turns out that there is an important, but subtle difference between last_save_time and save_interval. One is a point in time. The other is a duration.
So, after dumping all the C library time keeping (I use the term loosely here) garbage, you might write this:
template <class DataType>
class MemDB {
// ... [snip] ...
std::steady_clock::time_point last_save_time;
std::steady_clock::duration save_interval;
// Note I just used the default duration for std::steady_clock for the
// save interval. You could use a specific duration type instead, like
// storing the duration as std::chrono::seconds, if you like.
// ... [snip] ...
void save() {
// ... [snip] ...
last_save_time = std::steady_clock::now();
}
// ... [snip] ...
MemDB(DataType* data, std::steady_clock::duration save_interval = std::chrono::seconds(300))
// ... [snip] ...
, last_save_time{std::steady_clock::now()}
, save_interval{save_interval}
{}
// ... [snip] ...
void idle() {
if (data_changed && ((std::steady_clock::now() - last_save_time) > save_interval))
save();
}
// ...
Which, as you can see, is more or less identical to the C library implementation… except better in every way.
(Note that if you really, really must use the C library, std::clock() may be a better option than std::time(). std::clock_t’s resolution and such will still be a mystery, but std::clock() may be steady. (I can’t confirm this, and don’t care to research it.))
void save() {
std::lock_guard<std::mutex> lock(mtx);
data->save();
data_changed = false;
last_save_time = time(0);
}
Not sure of the logic of making this function private. Why hide it?
MemDB(DataType *data, time_t save_interval = 300)
: data_changed(false), last_save_time(0), save_interval(save_interval)
{}
You have a pretty glaring bug here that should have been caught very easily had bothered not only to test your code, but even just compiling it with warnings on. See what’s missing?
virtual ~MemDB() {
save();
}
Eeeeh…. Okay, this seems pretty risky, if DataType::save() isn’t noexcept… and it’s hard to imagine that it could be.
In a realistic use case, I would expect that if saving the data failed, I’d get an error. That’s kinda important. If the save fails, and nothing informs me… that’s bad.
Okay, so DataType::save() throws on fail, which means I expect that if I manually save, via idle() or via reader()->save(), I can expect an exception. All good.
But that means this destructor is a time bomb. And worse, it’s a time bomb I can’t defuse.
Saving in the destructor isn’t necessarily bad if you give the use the option of doing it in two phases. If I want to be sure the data is properly saved, I would like to be able to do something like:
// assuming: auto data = MemDB<whatever>{&some_data};
// I've done everything I wanted to do, now time to clean up:
data.save(); // unconditionally save (idle() would be silly, because it might not save)
// data.~MemDB<whatever>(); // safe, because it won't try to save, because I already did
But there’s just no way to do this with the current API.
- There’s no way to unconditionally save, because for some bizarre reason,
save() is private. All I’ve got is idle() which may not save… and I’d have no way to know. (I mean, technically I could do data.reader()->save()… but that’s silly.)
- There’s no way to stop the destructor from unnecessarily locking and saving and doing all the other time-checking gymnastics.
I mean, the fix is simple:
~MemDB()
{
if (data_changed.load(std::memory_order::acquire))
{
try
{
auto lock = std::scoped_lock{mtx};
data->save();
}
catch (...)
{
// do nothing
}
}
}
(The try block is optional, but given that data->save() should be considered high-risk, I think it’s a good idea.)
void idle() {
if (data_changed && time(0)-last_save_time > save_interval)
save();
}
This is fine, I guess, but it would probably be handy to return a bool indicating whether or not a save actually happened. Why throw away useful information?
template <class AccessDataType>
class Accessor {
std::lock_guard <std::mutex> lock;
AccessDataType data;
So, the intention here is that whenever you have MemDB<T>, then AccessDataType is either T* or T const*. AccessDataType will never be U* or T& or anything else. Only T* or T const*.
So writing the type this way is too permissive. It’s also too verbose, because now you have to write MemDB<T>::AccessDataType<T const*> or MemDB<T>::AccessDataType<T*>. It would be much nicer to be able to write MemDB<T>::ReadAccess or MemDB<T>::WriteAccess.
An easy way to do this without code duplication is simply:
template <class DataType>
class MemDB
{
private:
template <bool Const>
class AccessT
{
private:
using DataTypePtr = std::conditional_t<Const, DataType const*, DataType*>;
std::lock_guard<std::mutex> lock;
DataTypePtr data;
AccessT(std::lock_guard<std::mutex> lock, DataTypePtr data)
: lock{std::move(lock)}
, data{data}
{}
public:
// Moveable.
AccessT(AccessT&&) noexcept = default;
auto operator=(AccessT&&) noexcept -> AccessT& = default;
// Non-copyable.
AccessT(AccessT const&) = delete;
auto operator=(AccessT const&) -> AccessT& = delete;
// If you're restricting *some* fundamental ops, it's a good idea to
// explicitly enable the ones you're allowing.
~AccessT() = default;
auto operator->() const noexcept -> DataTypePtr { return data; }
friend class MemDB;
}:
public:
using ReadAccess = AccessT<false>;
using WriteAccess = AccessT<true>;
auto reader() -> ReadAccess
{
return ReadAccess{std::lock_guard<std::mutex>{mtx}, data};
}
auto writer() -> WriteAccess
{
auto lock = std::lock_guard<std::mutex>{mtx};
data_changed.store(true, std::memory_order::release);
return WriteAccess{std::move(lock), data};
}
// etc.
};
The constructor for the inner class is private, so we make the outer class a friend. Now the only way for users to create one of the access objects is via the reader() or writer() function.
Note that I’ve enabled the move ops, which are implicitly deleted in your version. Things shouldn’t even compile using your version in C++11… what’s likely happening is (assuming you tried to compile it at all), you accidentally compiled it in C++17 mode, which mandates RVO, which means you could return your access objects without needing move ops. (Which, really, is the universe telling you to move on from C++11, and use more modern standards.)
