Fix thread-safety when calling request_resize()

[directmusic]

This patch checks if calls to request_resize() happen on the main UI thread, and if not, sets a flag and stores the requested size so ClapAsVst3::onIdle() can handle the request.

[SamWindell]

Oh yes we certainly need this, though we can't have 3 separate variables for this, the whole thing needs to be a single atomic operation. On modern CPUs we have 64 bits for atomics, so I suggest we could have something like this:

struct GuiResizeRequest {
    bool operator==(GuiResizeRequest const& other) const = default;
    uint32_t w, h;
};
constexpr GuiResizeRequest invalid_size = {(uint32_t)-1, (uint32_t)-1};

// Member of WrappedView:
std::atomic<GuiResizeRequest> resize_request {invalid_size};

// When a request is given on the non-main thread:
resize_request = GuiResizeRequest {w, h};

// onIdle:
if (auto const size = resize_request.exchange(invalid_size); size != invalid_size) {
    // Do resize
}

Actually do we need to support 32-bit CPUs/builds? Though I guess std::atomic uses a mutex if it doesn't support the size lock-free so it would be safe even then.

[geraintluff]

I'm definitely doing 32-bit builds with clap-wrapper in general, but not VST3. I've heard of some pedals running VST3s on embedded Linux, so I wouldn't rule it out, but it's also not one of the ones we (currently) check in CI.

However, the default assignment/conversion for atomic_bool is memory_order_seq_cst. To the best of my knowledge, this means that if we assign the atomic_bool last, then any thread reading that value is also guaranteed to see the width/height changes.

[SamWindell]

Ok good to know. I think my worry is misguided because it still should work great with 32-bit, it's just behind the scenes a mutex may be used instead of atomic assembly operations. That's fine in this case.

You're right about the memory_order_seq_cst, but in this case the more important aspect is that it's undefined behaviour to read a variable that another thread may be writing; request_resize could be called again at the same time we're handling the first request. We could make them their own atomic to fix that, but we can't guarantee width and height will be available simultaneously to another thread, even with seq_cst. So it all needs to be done as one operation.

[defiantnerd]

I'd suggest to swap lines 1298 and 1299, first reset the flag, the call request_resize().
Perhaps we need an even stronger thread protection but this should be good for now.

[SamWindell]

Either way it is undefined behaviour!

Different threads of execution are always allowed to access (read and modify) different memory locations concurrently, with no interference and no synchronization requirements.

Two expression evaluations conflict if one of them modifies a memory location or starts/ends the lifetime of an object in a memory location, and the other one reads or modifies the same memory location or starts/ends the lifetime of an object occupying storage that overlaps with the memory location.

A program that has two conflicting evaluations has a data race unless

• both evaluations execute on the same thread or in the same signal handler, or
• both conflicting evaluations are atomic operations (see std::atomic), or
• one of the conflicting evaluations happens-before another (see std::memory_order).

If a data race occurs, the behavior of the program is undefined.

https://en.cppreference.com/w/cpp/language/multithread.html

It's one of those things that's fine in 99.9% of cases, but why not just do it so its 100% safe?

[baconpaul]

Why not use the proper atomic compare and exchange in the if

https://github.com/baconpaul/six-sines/blob/e4b3e447449628adf5175daf3df9f54b2aa68314/src/synth/synth.cpp#L780

there’s an example

[baconpaul]

I've solved the GitHub issue; we just need to rebase next these prs.

git checkout next
git pull upstream next
git checkout my-branch
git rebase next my-branch
git push origin my-branch --force

is the common pattern if origin is set to your fork and upstream is set to free-audio/clap-wrapper

[directmusic]

@SamWindell's solution is much more robust and I would feel more comfortable going with that. I'm not sure what the open source etiquette is regarding replacing my code with a better solution from discussion in a PR. If that the step forward, should I push my commit with credit to @SamWindell in the commit message?

[SamWindell]

I'm no expert on these things either. But perhaps in this case if you're happy to swap your current atomic code with my suggestion that's perfect with me. No credit needed unless you want another name on it for future reference. Entirely up to you.

After you make the change you could squash the commits so the intermediate step is gone and then git push --force. It's not essential though I don't think.

Also see Paul's request so this PR uses the new fix for the Windows CI that are currently showing failed!

[baconpaul]

Yeah I can add multiple authors when I merge so collaborate on this pr and we can give it correct attribution at squash time in GitHub

[directmusic]

Just updated the code. I am seeing the issues in the build. I have a bit of time-sensitive work to do today, but I will address those issues asap and get this passing CI. Thanks for all the help

[defiantnerd]

This looks good for me.

[baconpaul]

Ugh I hate to be a pain

Atomic is specialized for fundamental types like bool and int to be non locking. This is guaranteed for all types

Do we care here though? All of these threads are ui threads not audio threads. If so that's fine and we can merge - if not we need to static sssert that atomic is lock free is true

[baconpaul]

Looking it does all seems ui thread so I think it's indeed good

But just want to sanity check once more

[defiantnerd]

you're right, but checking this in compiler explorer:

#include <atomic>
#include <iostream>
#include <utility>
#include <cstdint>
 
struct A { int a[100]; };
struct B { uint32_t x, y; };
 
int main()
{
    std::cout << std::boolalpha
              << "std::atomic<A> is lock free? "
              << std::atomic<A>{}.is_lock_free() << '\n'
              << "std::atomic<B> is lock free? "
              << std::atomic<B>{}.is_lock_free() << '\n';
}

gets me for all relevant compilers:

std::atomic<A> is lock free? false
std::atomic<B> is lock free? true

so... we can add this for now, but we could still come up with something better

[directmusic]

By all means if you all are not confident on this solution do not merge it. I'm happy to use my branch for my projects in the meantime.

Checking if the audio thread is calling the function and telling the user they are doing something stupid using a ring buffer in that case could address that case. And/or adding in a case where we cast down from uint32_t to uint16_t on 32 bit builds so we can have a chance of the operation being lock-free on those systems is a possible solution for that issue.

If you all know of a MPSC queue that is licensed appropriately for this project I'd be happy to integrate that.

[defiantnerd]

I tried to write a static_assert but unfortunately is_lock_free() isn't constexpr :(

If we agree that no one will open a surface of width or height of 65536 or higher we can mix that into a single uint32_t and atomic that.

[baconpaul]

Or make the item have a pair of atomics rather than being an atomic of the struct.

[baconpaul]

Ahh request_resize is marked thread_safe so yeah this shouldn't lock.

I think indeed the trick is to make the struct stay as is but have a std::atomic<uint64_t> which we pack as width << 32 + height and make it zero when read. So do the compare_exchange with 0 atomically etc...

small change but probably one worth making.

[directmusic]

@defiantnerd you should be able to statically assert against is_always_lock_free in C++17.

@baconpaul When checking cppreference I came across this line "All atomic types except for std::atomic_flag may be implemented using mutexes or other locking operations, rather than using the lock-free atomic CPU instructions." It seems like no matter the type it could lock.

[baconpaul]

@SamWindell yes that is true.
but on ARM and x86 with modern compliers they are specialized for int bool etc.... to use the lock free memory isntructions.
I guess its a risk that, say, riscv compilers don't implement this and the vst3 request resize from the audio thread locks.
I kinda think we should cross that bridge when we come to it though. All our lock free mechanisms assume atomic int and so on work.

I think it would be handy to at least dump a warning if atomic is not lock free at runtime.

[SamWindell]

Hmm oh yes. I guess it's technically allowed to call resize_request from the audio thread. So we do need to ensure lock-free.

The static assert against is_always_lock_free would be my suggestion too.

And cross the bridge if people are hitting that error. We'd probably have to cast down to u16.

Do we need to support older than C++17?

Having separate atomics for width and height wouldn't be an ideal solution. As far as I know, no matter what memory_order you use, you can't guarantee the UI thread will see separate width and height atomics at the same time. It could be possible that you could see the width from one resize_request and the height from another.

[defiantnerd]

IIRC we have minimum C++17 for the wrapper. Right now, I would prefer the u16u16_t variant. I would merge this to next and update it accordingly.

Okay?

[SamWindell]

Fine with me if you want to ensure this works for the CPUs that only support 32-bits for atomics lock-free.

One thing to look into with casting down to u16 - are we allowed to reject the resize_request by returning false? If the sizes don't fit in u16 we could reject rather than have a truncation error.

[defiantnerd]

Yeah, that is a good idea. Return false if nonsense coords.

[defiantnerd]

I have refactored this here: #411.