Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Key/Button::is_pressed are not thread safe #226

Open
crumblingstatue opened this issue Sep 1, 2020 · 4 comments
Open

Key/Button::is_pressed are not thread safe #226

crumblingstatue opened this issue Sep 1, 2020 · 4 comments

Comments

@crumblingstatue
Copy link
Collaborator

At least on X11, it can trigger all kinds of errors to call them from a different thread.
And someone messaged me that it can cause segfaults for them to do so.

@memoryleak47
Copy link
Contributor

One partial solution to this would to add a dummy &RenderWindow parameter to Key::is_pressed. This should prevent it from being called in another thread than where the RenderWindow lives, because RenderWindow is !Sync.

This wouldn't work whenever the user creates multiple RenderWindows though.

@crumblingstatue
Copy link
Collaborator Author

Looks like is_pressed isn't the only misbehaving API. There can be threading related errors for example if you create 2 sf::RenderWindows on 2 different threads. And there is probably more.

I don't have any clear plan on how to solve all of these threading issues, so for the time being I just added a warning to the documentation.

@memoryleak47
Copy link
Contributor

What about adding a static unique_threadid: OnceCell<ThreadId> containing the thread id of the thread in which a RenderWindow is spawned.
And in RenderWindow::new() one

  • sets unique_threadid to thread::current().id() if unique_threadid.get().is_none()
  • does nothing if unique_threadid.get() == Some(thread::current().id())
  • returns an error / panics, if the unique_threadid.get().is_some() but contains another thread-id than thread::current().id().

Using this, every RenderWindow has to live in the same thread.

This then allows to either:

  • let non-thread-safe functions panic / error if they are called from the wrong thread (by comparing thread::current().id() to unique_threadid)
  • add a dummy &RenderWindow argument to every non-thread safe function, which implies that this function is only called from the unique_threadid-thread.

@crumblingstatue
Copy link
Collaborator Author

That sounds like it might work. If you'd like you can submit a pull request, but for now I'll try to explore what other thread safety issues we might have.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants