[Discussion]: Get rid of global member variables

[Krzmbrzl]

Currently there are multiple places in which classes expose some of their member variables as public allowing direct read/write access to it. This however (in my opinion) makes it very easy for a multi-threaded application like Mumble to produce data-races and similar stuff. The solution for this (I have encountered in the source) is to add a public lock-object either for the whole class or on a per-variable-basis that has to be locked before using the variable.
Although this perfectly resolves the above-mentioned problem , it still appears quite error-prone to me as the use of the lock is not enforced. Thus I could go ahead, use/change that variable without acquiring the lock first which eventually will most likely result in some nasty bugs in the application.

One solution to this problem would be to make all member variables protected or private and supplying public getter and setter methods for them. Then these methods would implement the locking and once that is done properly, no one will be able to access these fields without locking them first (except of course he/she is working on the class itself but that is something you can't protect against).
Now I know that getters and setters are not everyone's favorites but I recommend reading through https://stackoverflow.com/questions/760777/c-getters-setters-coding-style for some arguments in its favour.
It's not all sunny though. Here are the disadvantages I can think of:

• Having getters and setters for every previously public member variable brings quite some code-overhead with it potentially cluttering the source
• Extra function invocations bring a tiny little performance-overhead with them (though I'd say that we can neglect that in our case)
• There might be code that accesses these fields in a loop. Right now it can aquire the corresponding before entering the loop and then the loop can run through undisturbed. If the loop had to use a getter/setter method instead, the lock had to be re-aquired every iteration potentially leading to a big runtime overhead of constantly re-acquiring the same lock over and over again
• Quite some effort to implement this into the existing code-base

Another solution to this could be to rework how we get a hold of these classes. Currently this (often) happens via a static get method returning the respective class instance that is then used to access all its fields. If instead we returned a (class-specific) wrapper instead, we could implement that one in such a way that it will automatically lock the respective locks in that class.
In essence I have something syntactically similar to a smart-pointer in my mind. Instead of returning the pointer to that instance, we return Wrapper<MyClass> which has an overloaded -> operator allowing the access to the underlying instance. It will acquire all locks in its constructor and release them in its destructor though.
In order to decide whether a read- or a write-lock has to be acquired, an extra parameter has to be given to the respective get method (e.g. ClientUser::get(Access::Read)).
Pros:

• A lot less code-overhead => less cluttering in the source
• Potentially easy to integrate into the existing code-base as the accessing itself doesn't have to be changed (assuming the existing accessing happens on pointers already)

Cons:

• Unspecific. An object might have multiple locks for different fields that could be locked independently. With this approach its all-or-nothing
• Implicit locking. A new programmer might acquire a lock this way without even noticing, leading to locks being held for longer than necessary and potentially accidental dead-locks that this particular programmer (at first) won't recognize as such as he/she apparently didn't use any locking mechanism to begin with.

All in all though I think it is somewhat important to choose a way of enforcing locking (be it either of the two solutions or something you can come up with) as this (I think) makes the Mumble code more future-proof and in general more sophisticated.
Another bonus of this would be that it'd be easy to tell if a certain method is thread-safe or not. As of now there are a bunch of methods I don't know this, because they (apparently) access variables without holding a lock. Is this because no other code uses that variable or is this because that variable is not meant for multi-threaded access? I can't tell. If variable-access was lock-protected by default, this would be a lot easier to tell.

Another thing to consider here is that with the new plugin framework (#3743, #3742) a whole new layer of complexity is added to the underlying problem as the plugin's code is out of control for the Mumble-devs. Thus it might spawn new threads that access some data fields via the API. Thus a strategy of "we know what variables we access when, thus we don't need to lock them" doesn't hold (anymore). This of course implies that my PR will actually get merged into master at some point but if this weren't the case I hope someone would have stopped it by now xD

[Krzmbrzl]

Oh another solution I just came about: Use std::atomic or otherwise atomic types.

For more info see https://stackoverflow.com/questions/31978324/what-exactly-is-stdatomic

[davidebeatrici]

Thank you very much for the very detailed info!

Using atomic types is definitely the best solution, in my opinion.

[Kissaki]

I’m not reading the full text now, but from the topic: Getting rid of the global state/variables has always been a wish and goal, at least ever since I have been here, so at least 10 years.

It is not a question of desire or acceptance but of effort, time resource and priority.

[davidebeatrici]

Indeed.

At least Mumble doesn't seem to have serious race conditions, Murmur on the other hand (see #3893)...

[Krzmbrzl]

The problem with atomics that I see is in a situation like this:

if (myValue.load() == Something) {
    // Do some stuff
    
    myValue.store(SomethingElse);
}

In this example myValue could still change while the code block is doing "stuff" as there is no lock that is held throughout the whole code-block. This might or might not lead to problems, but I think it is definitely something to think about...

[Krzmbrzl]

This is what #4195 is about (not exclusively though). No need to have 2 issues for the same thing