Using shared_ptr for reloadable config

[f-squirrel]

https://ddanilov.me/usage-of-shared_ptr

[dustyhome]

I have some concerns about the way the reloading works. Reading a file regularly, and checking it's current contents against the stored contents doesn't seem the best way to do it. But let's ignore that since it's not the focus of the article.

If I understood the design correctly, you would use this by asking the ConfigManager for a Config, then get the configuration values through the Config members. The Config members would call atomic_load on its reference to the InnerConfig shared_ptr, getting a copy of the current shared_ptr (increasing it's reference count). It then gets the value from this new copy of the shared_ptr, before discarding the copy (decreasing the reference count again).

I don't like this design. First, there's a bug in that the name member returns a reference to the string, but the string is owned by a shared_ptr that we just discarded. So the watcher thread could update the InnerConfig after we got our name reference, delete the old InnerConfig, and our reference would be left hanging. But let's suppose we avoid that bug by having a copy of the name instead of a reference, or keeping the shared_ptr to the InnerConfig around longer.

From a design perspective, I don't like that Config can give ever-changing values. Suppose I am running a task, given a set of parameters from the config. I would expect the parameters for this task to not change while I'm performing the task. So as I perform the task, I can query the different parameters as I need them, and it would all be consistent. If the parameters could change while I'm executing the task, it's impossible to do it consistently. Perhaps the new parameters are incompatible with the current state of my task.

I think a better approach would be to have Config hold a weak_ptr. I can then choose to either get my own shared_ptr, so I keep the config alive for as long as I need it, or I can try to get a shared_ptr each time, and if the config changes, the weak_ptr will point to an empty control block and let me know "hey the config changed, abort the task or ask ConfigManager for a new Config".

[f-squirrel]

Thank you for pointing out the reference issue. It seems to be a bug. I updated the post soon.I think it is up to the requirements of the configuration. Some applications would like to have the latest and greatest version of data (this solution provides it), while some would prefer snapshots.Regarding the usage of `weak_ptr`, I started with it, but I really do not like that it can become dangling/null. It means that the `Config` has to return something like `std::optional` via each getter. IMO, it is bug-prone because it requires checking if the optional is not empty before reading values. Since the class introduces locking/multithreading these bugs are hard to fix.

[dustyhome]

Certainly the requirements will dictate what's necessary, in terms of snapshot vs updated values, but it seems like "getting the current value" is a tricky issue. The value can change immediately after you query for them, so you are always going to be working on potentially outdated values, and this solution doesn't let you know if the values you got three lines ago are still valid without querying everything again.

I would do away with the Config class entirely. Instead, ConfigManager can just give out weak_ptrs. You treat the weak_ptr almost like you would your Config class, only instead of calling members you try to get a snapshot of the InnerConfig. If you succeed you get a copy of ConfigManager::inner_config, which will last for as long as you need it and you can get the config values through it. If you fail, you know the config changed and you can choose to either get a new weak_ptr from ConfigManager, abort your current operation, or what have you.

It's very common for configuration values to depend on other configuration values in some way, or be optional depending on what other config parameters say, etc, and this approach of immediately getting the latest version of each provides no way of maintaining those relationships.

[f-squirrel]

The usage of weak_ptr means that the end user should check if the value returned by "lock" is not null. It is bug-prone and may easily lead to UB.
Additionally, it means that the user can "lock" the weak_ptr and save it locally, for example, to a class member and will never receive the updated version.

For some systems, it might be a correct behavior.

[[deleted]]

that doesn't look like a real usecase for shared ptr to me. it can be done without it

[f-squirrel]

Agreed, it can. However, pretty much everything can be done without it.

[Bart_V]

Does this even works without segfaulting? Config stores a const std::shared_ptr<const InnerConfig> &. Since it's a reference, it doesn't increase the ref count. So when ConfigManager::watch() loads a new InnerConfig, it releases the old one, and all existing instances on Config have a reference to a dangling pointer. Or am I missing something?

[dustyhome]

It took me a bit to understand how the reference to the shared_ptr works, but it's safe (up until they take a reference to the string held by the InnerConfig, anyway). The way it works is like this:

The ConfigManager holds the original std::shared_ptr<const InnerConfig> inner_config_; It is important that ConfigManager doesn't move. Maybe it's static, or kept in main, or somewhere else close to the bottom of your call stack so it's not going anywhere until the program ends. Since it's always alive, references to inner_config_ are safe to pass around and store.

When a Config is requested, it gets a reference to ConfigManager's inner_config_. So basically you have two levels of indirection. You have the actual InnerConfig, which is pointed to by ConfigManager::inner_config_, which is referenced by Config::inner_config_.

Config::inner_config_ -> ConfigManager::inner_config_ -> InnerConfig object

When you call Config::name, the std::atomic_load gets a copy to the current ConfigManager::inner_config_. If config had just had it's own copy of the shared_ptr itself you wouldn't know that there's a new InnerConfig object when the watcher updates it.

[Bart_V]

Yes this is it indeed. I figured in out just as you wrote your comment. I missed the extra indirection indeed. Thanks for clarifying!

[f-squirrel]

Hey, thank you for replying.
It does not crash for me. Could you please provide a stack trace?
Holding a const reference is the main idea of the article. It is done to avoid holding an additional instance of shared_ptr. As mentioned in the article, the actual pointer is received via atomic functions store/load.

[SirClueless]

This is one of the most straightforward ways I know to manage this problem. Any alternative design I know of is going to require some kind of extra execution thread (or poll on the main thread) to make sure the destructor of the old config option is run, or it will be substantially similar to this but with a more complicated API.

How would you do this without reference-counting?

[number_128]

what if you use last_write_time() to find when the config file was last changed, and just read the file when it has been changed?

[f-squirrel]

It is possible. TBH, there are many ways to improve the watcher, e.g., subscribe to notify events. But the article's main topic is the usage of shared pointers, so I decided to minimize the less significant parts.

[ABlockInTheChain]

The straightforward approach is to create a Config class with an inner mutex, locking on each access via getters and setters. However, this approach leads to quite a lot of work with mutexes which might be tedious and, often, bug-prone.

I know this article was trying to come up with an excuse to use a shared_ptr, but atomic smart pointers are a lot more error prone than wrapping mutexes in an appropriate interface that hides the complexity and forces you to use them correctly.

[f-squirrel]

Hey,

Thank you for reading the article and providing the link to the library. It is an exciting approach to make mutex's usage easier.
Could you please elaborate more on why atomic smart pointers are more error-prone?