Avoid thread-unsafe spin-locks when running code on the main thread

[dominiklohmann]

Bug report

Currently, yabai executes code on the main thread by running dispatch_after.

Here's one of many times this is used in the code.

yabai/src/osax/payload.m

Lines 593 to 599 in 54a1050

	volatile bool __block is_finished = false;
	dispatch_after(dispatch_time(DISPATCH_TIME_NOW, 0), dispatch_get_main_queue(), ^{
	asm__call_move_space(source_space, dest_space, dest_display_uuid, dock_spaces, move_space_fp);
	objc_msgSend(dp_desktop_picture_manager, @selector(moveSpace:toDisplay:displayUUID:), source_space, dest_display_id, dest_display_uuid);
	is_finished = true;
	});
	while (!is_finished) { /* maybe spin lock */ }

Some notes on this approach.

1. volatile does not guarantee atomic access. A proper way to do this would be to use atomic_flag from C11 in the spin-lock or a condition variable to avoid the spin-lock completely.
2. There's a function called dispatch_async, which does the same as dispatch_after called with dispatch_time(DISPATCH_TIME_NOW, 0).
3. There's a function called dispatch_sync, which runs code immediately on the main thread, and then returns control afterwards, thus avoiding the issue from 1. completely.

It is better to leave the return of control to Grand Central Dispatch instead of doing it manually by spin-locking. This also avoids having to deal with issue 1. in the code sample above.

There is likely more code in yabai relying on the false assumption that volatile means thread-safe. This should be looked at in-depth. See also Why the "volatile" type class should not be used from kernel.org and this proposal for deprecating some uses of volatile in C++20.

If you want help with it I can send a PR converting all these throughout the code base.

[koekeishiya]

There's a function called dispatch_sync, which runs code immediately on the main thread, and then returns control afterwards, thus avoiding the issue from 1. completely.

Didn't know this. Should probably switch to this for clarity.

volatile does not guarantee atomic access. A proper way to do this would be to use atomic_flag from C11 in the spin-lock or a condition variable to avoid the spin-lock completely.

I'm aware of that. The purpose here is not thread-safety, however because the value is only written to from one place, and is also read from only one place, volatile is sufficient because it forces the compiler to ensure that the value is always read from memory and not from the cache. Because of this there is no reason to use atomics here. I do use atomics where I deem it necessary, see src/event_loop.c for instance.

[dominiklohmann]

The purpose here is not thread-safety, however because the value is only written to from one place, and is also read from only one place, volatile is sufficient because it forces the compiler to ensure that the value is always read from memory and not from the cache.

This is a tough problem to solve and while I'm not 100% certain, I'm pretty sure it's undefined behaviour as the CPU may reorder instructions with no memory barrier present.

volatile only guarantees that the read/write actually happens, but it does not prevent reordering.

Anyways... this should be solved by using dispatch_sync instead.

[koekeishiya]

I'm pretty sure it's undefined behaviour as the CPU may reorder instructions with no memory barrier present.

That is actually true in this scenario as there are no dependencies between these two statements and it is therefore not guaranteed that the value is written at the point where I wanted it to be.

I agree that the way to fix this is to simply use dispatch_sync here instead.

[koekeishiya]

Fixed on master.