Add a template specialisation for ScopedValueSetter<std::atomic<Type>>

[PluginPenguin]

I just noticed that ScopedValueSetter does not work with std::atomic types. Adding a template specialisation like this made it work. Would you consider adding it to the codebase?

/** Specialisation for a ScopedValueSetter referencing a std::atomic */
template <typename Type>
class ScopedValueSetter<std::atomic<Type>>
{
public:
    /** Creates a ScopedValueSetter that will immediately change the specified value to the
        given new value, and will then reset it to its original value when this object is deleted.
    */
    ScopedValueSetter (std::atomic<Type>& valueToSet,
                       Type newValue)
        : value (valueToSet),
          originalValue (valueToSet)
    {
        valueToSet.store (newValue);
    }

    /** Creates a ScopedValueSetter that will immediately change the specified value to the
        given new value, and will then reset it to be valueWhenDeleted when this object is deleted.
    */
    ScopedValueSetter (std::atomic<Type>& valueToSet,
                       Type newValue,
                       Type valueWhenDeleted)
        : value (valueToSet),
          originalValue (valueWhenDeleted)
    {
        valueToSet.store (newValue);
    }

    ~ScopedValueSetter()
    {
        value.store (originalValue);
    }

private:
    //==============================================================================
    std::atomic<Type>& value;
    const Type originalValue;
};

[reuk]

Would you mind providing a few more details about your use-case? Allowing the ScopedValueSetter to work with more types seems like a broadly good idea - however, in the specific case of atomic variables I’m not sure whether the ScopedValueSetter is the correct pattern to use.

I’m not sure exactly what you have in mind, but it sounds like you might be implementing a ‘guard’ of some kind that signals when it is safe or unsafe to modify another variable. If this is the kind of behaviour you’re looking for, I’d recommend using a SpinLock in combination with a ScopedLock or a ScopedTryLock instead.

---

It’s also worth pointing out that the first constructor might not behave as expected. There’s a chance that another thread might modify valueToSet between reading its value into originalValue and storing the newValue. It’s probably slightly more correct to do something like this, so that the new value is set and the old value read as a single atomic operation:

: value (valueToSet)
, originalValue (value.exchange (newValue)) {}

[daniel]

This is a great request, I would use it for any state that needs to be wrapped in atomic for whatever reason (likely being read by multiple threads).

std::atomic_bool isProcessing;

void process()
{
    ScopedValueSetter p (isProcessing, true);
    / ...
}

The operations need to be atomic, good catch

[PluginPenguin]

Thanks for your feedback. In contrast to a lock, my use case is that I schedule a task to derive some output data from some input data to a low priority thread pool. Once the derivation process is done, the thread pool task acquires the lock for the output data and writes it. There is, however a chance that the user manually restores the state from memory. In case a manual data restoration is in progress, I simply want to discard the computed values that the background task just computed instead of taking the lock and writing them because the results are considered outdated in that case. To do so, I have an atomic bool flag. So my implementation looks like this

void backgroundTask()
{
    // do a lot of time consuming computation
   
   if (dataRestorationIsInProgress.load())
      return;

   const juce::ScopedLock sl (dataAccessLock);
   // write computed data
}

void restoreData()
{
    juce::ScopedValueSetter<std::atomic<bool>> svs (dataRestorationIsInProgress, true);
   
   const juce::ScopedLock sl (dataAccessLock);
   // write restored data and do some more stuff
}

One could think of taking a try lock in the background task, but there are other reading threads involved, so that some thread is holding a lock is no good indication for a data restoration in progress.

Does this all make sense to you or am I over complicating things?

Good point however with the exchange operation!

[reuk]

@PluginPenguin

Does this all make sense to you or am I over complicating things?

Are backgroundTask and restoreData the only functions that can modify the data? If so, I don’t think there’s any need for a separate flag:

void backgroundTask()
{
    const juce::ScopedTryLock sl (dataAccessLock);
    
    if (! sl.isLocked())
        return;

    // If we were able to take the lock, that must mean that no other
    // state restoration was in progress. Write computed data.
}

void restoreData()
{
    const juce::ScopedLock sl (dataAccessLock);
    // Write restored data and do some more stuff
}

@daniel

This is a great request, I would use it for any state that needs to be wrapped in atomic for whatever reason (likely being read by multiple threads).

That’s my concern - that users would use this for whatever reason, even if it’s not a good fit. I can see that it might be useful in some limited scenarios (maybe if there’s a long-running process on a background thread, and the UI needs to poll the state of this process to display some sort of “working…” indicator). However, for anything more complex, where mutliple threads are attempting to synchronise access to some resource, I think that the ScopedValueSetter is probably a bad fit, and a proper mutex should be used instead.

[PluginPenguin]

@reuk

Are backgroundTask and restoreData the only functions that can modify the data? If so, I don’t think there’s any need for a separate flag:

As stated above, this won’t work because of other threads also taking the lock from time to time.

@PluginPenguin

One could think of taking a try lock in the background task, but there are other reading threads involved, so that some thread is holding a lock is no good indication for a data restoration in progress.

Of course, I could add a second CriticalSection instead of the std::atomic that is locked during restoreData and try locked in the background task, which would basically do the same job and result in the same count of code lines. But to me that would feel more like abusing the mutex, because from a logical point of view, my main concern is to be informed that something is in progress, rather than protecting data access.

[reuk]

@PluginPenguin

As stated above, this won’t work because of other threads also taking the lock from time to time.

Sorry, I need to read more carefully. Perhaps in this case something like a ReadWriteLock would be more appropriate. Your readers can all access the shared state simultaneously. restoreData always takes the writer lock (ensuring that no reads happen simultaneously). backgroundTask uses tryEnterWrite and aborts if the data is already being written.

If a ReadWriteLock is not a good fit, then I think a SpinLock around the restoreData call is probably the way to go. restoreData can lock the mutex to signal that it is in progress. backgroundTask can use a try-lock to attempt to lock the mutex, and abort if the mutex is already held by restoreData.

[reuk]

@PluginPenguin

from a logical point of view, my main concern is to be informed that something is in progress, rather than protecting data access.

I’m not sure I agree. It sounds like you’re trying to synchronise access to some data, specifically to ensure that only one function modifies the data at a time, with the option of aborting a write if another thread is already writing. This seems like a very reasonable place to use a lock of some sort.

[cpr2323]

Just in case someone wants a generic scoped object, I have a tiny little helper class that I find very useful, as it allows you to scope any operation. The JUCE team is welcome to add it. oh, after I posted this, I am wondering if maybe ‘exit’ and ‘scope’ are redundant? lol

class ExitScopeExecuter
{
public:
    ExitScopeExecuter (std::function<void ()> theExitFunction)
        : exitFunction { theExitFunction }
    {
        jassert (exitFunction != nullptr);
    }
    ~ExitScopeExecuter ()
    {
        exitFunction ();
    }

private:
    std::function<void ()> exitFunction;
};

[reuk]

In C++17, it’s two lines:

template <typename Fn> struct ScopeGuard : Fn { ~ScopeGuard() { Fn::operator()(); } };
template <typename Fn> ScopeGuard (Fn) -> ScopeGuard<Fn>;

// usage
const ScopeGuard scope { [] { std::cout << "goodbye world\n"; } };

[PluginPenguin]

Thanks for your thoughts. Indeed, a ReadWriteLock seems like a clean solution here. Didn’t think about using it in this context.

Regarding your ScopeGuard example, while I understand the first line, I fail to figure out what language feature is used in template <typename Fn> ScopeGuard (Fn) -> ScopeGuard<Fn>;? Could you explain that briefly?

[reuk]

@PluginPenguin

Regarding your ScopeGuard example, while I understand the first line, I fail to figure out what language feature is used in template <typename Fn> ScopeGuard (Fn) -> ScopeGuard<Fn>; ? Could you explain that briefly?

That’s a deduction guide for the “class template argument deduction” feature. There’s more info here.

[nicolasdanet]

Thus in this case:

const ScopeGuard scope { [] { std::cout << "goodbye world\n"; } };

is equal to:

{ std::cout << "goodbye world\n"; }

Isn’t it?

[reuk]

Sort of, but this:

int main() {
    ScopeGuard s{[] { std::cout << "goodbye\n"; }};
    std::cout << "hello\n";
}

prints:

hello
goodbye

[nicolasdanet]

I’m sure there is a legitimate usage of that. It was just an ironic comment. Yesterday i found an old Stroustrup’s paper in which i noted words i really like.

We should not spend most our time creating increasingly complicated facilities for experts, such as ourselves. We need a reasonably coherent language that can be used by “ordinary programmers” whose main concern is to ship great applications on time.

That put me into the mood to react to what i considered a RAII orgy.
Sorry for the noise.
Your trick is very good.

< https://www.stroustrup.com/P0977-remember-the-vasa.pdf >

Notice that i consider myself as an “ordinary programmer”.