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

[WIP] thread safety #227

Merged
merged 2 commits into from
Sep 9, 2020
Merged

Conversation

memoryleak47
Copy link
Contributor

This adds the thread_safety::assert_window_thread-function, which panics if called from some other thread than the window-thread.
There is still a lot of room for experimentation in order to find out what functions should call thread_safety::assert_window_thread, or whether this mechanism is too restrictive or relaxed.
cc #226

@crumblingstatue crumblingstatue self-requested a review September 8, 2020 07:28
@crumblingstatue
Copy link
Collaborator

crumblingstatue commented Sep 8, 2020

This simple example still produces varying errors

    std::thread::spawn(|| {
        let mut rw = RenderWindow::new((640, 480), "Hi", Style::CLOSE, &Default::default());
    });
    println!("{}", Key::K.is_pressed());

This is likely because is_pressed() also inits some kind of windowy context (opens a connection to the X server on Linux at least), and it can run before the code in the thread::spawn above it, so it really should also set the window thread as well. Probably almost everything should set the window thread instead of just merely asserting it.

@crumblingstatue
Copy link
Collaborator

Do you have any objection to just getting rid of assert_window_thread entirely and just using set_window_thread everywhere?

diff --git a/src/thread_safety.rs b/src/thread_safety.rs
index 96c1e66..894cb77 100644
--- a/src/thread_safety.rs
+++ b/src/thread_safety.rs
@@ -12,20 +12,8 @@ pub(super) fn set_window_thread() {
         None => WINDOW_THREAD.set(current_id).unwrap(),
         Some(id) => {
             if *id != current_id {
-                panic!("Opening Windows in multiple threads is not supported.")
+                panic!("Graphics/windowing functionality is only supported on the same thread.")
             }
         }
     }
 }
-
-// asserts that the either current thread is WINDOW_THREAD, or that WINDOW_THREAD is unset
-pub(super) fn assert_window_thread(func_name: &str) {
-    if let Some(&id) = WINDOW_THREAD.get() {
-        if id != current().id() {
-            panic!(
-                "{} has to be called in the same thread as the RenderWindow!",
-                func_name
-            );
-        }
-    }
-}
diff --git a/src/window/keyboard.rs b/src/window/keyboard.rs
index 0da0417..b5c3552 100644
--- a/src/window/keyboard.rs
+++ b/src/window/keyboard.rs
@@ -127,7 +127,7 @@ impl Key {
     /// triggered.
     #[must_use]
     pub fn is_pressed(self) -> bool {
-        thread_safety::assert_window_thread("Key::is_pressed()");
+        thread_safety::set_window_thread();
 
         unsafe { ffi::sfKeyboard_isKeyPressed(self as _) }.to_bool()
     }
diff --git a/src/window/mouse.rs b/src/window/mouse.rs
index ca9aa57..b4b440c 100644
--- a/src/window/mouse.rs
+++ b/src/window/mouse.rs
@@ -77,7 +77,7 @@ impl Button {
     /// triggered.
     #[must_use]
     pub fn is_pressed(self) -> bool {
-        thread_safety::assert_window_thread("Button::is_pressed()");
+        thread_safety::set_window_thread();
 
         unsafe { ffi::sfMouse_isButtonPressed(self.raw()) }.to_bool()
     }
diff --git a/src/window/video_mode.rs b/src/window/video_mode.rs
index 6286ea4..9495f8f 100644
--- a/src/window/video_mode.rs
+++ b/src/window/video_mode.rs
@@ -79,7 +79,7 @@ impl VideoMode {
     /// return the urrent desktop video mode
     #[must_use]
     pub fn desktop_mode() -> Self {
-        thread_safety::assert_window_thread("VideoMode::desktop_mode()");
+        thread_safety::set_window_thread();
 
         unsafe { Self::from_raw(ffi::sfVideoMode_getDesktopMode()) }
     }

@memoryleak47
Copy link
Contributor Author

Sure, go ahead!

@crumblingstatue
Copy link
Collaborator

crumblingstatue commented Sep 8, 2020

I meant you could do it, unless you want me to make another pull request based on this, and close this one.

Although I guess I could submit a pull request to your fork, lol.

@crumblingstatue
Copy link
Collaborator

Thank you, I'll merge this as a good first step towards thread safety

@crumblingstatue crumblingstatue merged commit 7b0e379 into jeremyletang:master Sep 9, 2020
@memoryleak47
Copy link
Contributor Author

Merging this is maybe a little early;
As we are only certain that this problem arises when used with X11, I could image that a fair amount of non-X11 users which use multi-threading will now have their code broken.
Maybe we should add #[cfg(target_os = "linux")].

@crumblingstatue
Copy link
Collaborator

crumblingstatue commented Sep 9, 2020

That would mean that the application they made would work on non-linux platforms, but panic on Linux. Shouldn't the panic be universal so they design their application in a way that works on all platforms?

SFML is meant to be cross-platform and work as similar as possible on all platforms.

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

Successfully merging this pull request may close these issues.

2 participants