libsdl-org / SDL

Simple Directmedia Layer
https://libsdl.org
zlib License
9.86k stars 1.83k forks source link

SDL_clipboard thread safety documentation might be too restrictive #11325

Open Susko3 opened 1 week ago

Susko3 commented 1 week ago

Recently, thread safety documentation has been added to all SDL_clipboard functions that mentions "You may only call this function from the main thread."

In my usage, I've found that these APIs (at least the text APIs) work just fine when called from a non-main thread. Tested on Windows and Android.

Why are these APIs marked as only working from the main thread when they work just fine on some platforms? Is there a specific platform that requires it this way?

Would it be possible to mark these APIs as thread unsafe, while still allowing them to be called from any thread?

icculus commented 1 week ago

My assumption is that they won't work on some platforms, so I made an arbitrary call there.

We can change that if it's too aggressive, though.

Susko3 commented 1 week ago

From the OS/platform standpoint, I think it doesn't really matter which thread gets/sets the clipboard. It's going to take a global lock anyway, so there's no need to synchronize on the main thread.

We can change that if it's too aggressive, though.

Would be a welcome change. Currently, it's complex to get the clipboard from a non-main thread. The other thread has to "send an event" requesting the clipboard, then the main thread gets the clipboard, "posts" an event to the other thread with the clipboard content, and finally the other thread can consume the content.

icculus commented 1 week ago

So, at the moment, there's no lock, global or otherwise, and the even before we get to the platform layer, we already have race conditions.

Susko3 commented 1 week ago

I'm not expecting the SDL code to be thread safe. I'm expecting the function to work as expected when it's consistently called from a non-main thread. For example, a multi-threaded game using the clipboard from an "update" thread (that usually runs the game logic).

The solution I'd like to see is: "\threadsafety This function is thread-unsafe." or your standard copy for that.

slouken commented 1 week ago

The solution I'd like to see is: "\threadsafety This function is thread-unsafe." or your standard copy for that.

We can certainly do that. Keep in mind that the functions do things like talk to the window server on macOS and the X server on Linux, so you might really need to call these functions on the main thread.

icculus commented 6 days ago

Unlike my previous rant about Emscripten proxying everything, clipboard stuff would actually be a totally reasonable thing to proxy to the main thread if necessary, if a specific platform needs it. It's a rare event that an app would be forgiven for skipping a frame during.

Let's leave this open and in 3.2.0, and at least change the docs to say "thread safe" even if we have to later fix a platform.

Susko3 commented 6 days ago

Keep in mind that the functions do things like talk to the window server on macOS and the X server on Linux, so you might really need to call these functions on the main thread.

You may me right about macOS, people online say querying clipboard images from a dispatch thread may crash. OTOH, from our testing, accessing text is not a problem.

proxy to the main thread if necessary, if a specific platform needs it. It's a rare event that an app would be forgiven for skipping a frame during.

Does this mean that you would block a thread calling clipboard functions until the next SDL_PumpEvents() call from the main thread?

icculus commented 6 days ago

Does this mean that you would block a thread calling clipboard functions until the next SDL_PumpEvents() call from the main thread?

That's my thought, but only a) if we aren't the main thread already and b) the platform needs it. Like, if this already works well on Windows from a background thread, we wouldn't dispatch to the main thread.

I wouldn't recommend this as a general approach for anything that requires a main thread, but for getting/setting the clipboard, the overhead of waiting for the main thread to pump events is probably acceptable as a one-time thing that the user (probably) explicitly requested.