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

Warn on macos when initializing from non_main_thread #901

Merged
merged 3 commits into from Oct 20, 2019

Conversation

@nipunn1313
Copy link
Contributor

nipunn1313 commented Oct 18, 2019

Saw that we could do this with pthread, but opted to check the threadname as it was easier to do.
https://users.rust-lang.org/t/guis-and-the-main-thread/2863/22

I also wasn't sure if it belonged in fn init or fn set_initialized - tossed it in init as initial approach.

@nipunn1313

This comment has been minimized.

Copy link
Contributor Author

nipunn1313 commented Oct 18, 2019

issue from http://gtk-rs.org/docs/gtk/#threads

tested on our own app - saw the panic on mac!

src/rt.rs Outdated
@@ -89,6 +90,9 @@ pub fn init() -> Result<(), glib::BoolError> {
return Ok(());
} else if is_initialized() {
panic!("Attempted to initialize GTK from two different threads.");
} else if cfg!(target_os = "macos") && thread::current().name() != Some("main") {

This comment has been minimized.

Copy link
@sdroege

sdroege Oct 18, 2019

Member

There's nothing preventing you from starting a new thread with the name "main" :)

Ideally you'd use the pthread_main_np() API here, that's more reliable. You don't have to depend on some external crate for that, just do

extern "C" {
    unsafe fn pthread_main_np() -> i32;
}

And then call that in here

This comment has been minimized.

Copy link
@nipunn1313

nipunn1313 Oct 19, 2019

Author Contributor

Sounds good! Thanks

…which holds unsafe code
@nipunn1313 nipunn1313 force-pushed the nipunn1313:macos_warning branch from f3e3564 to dac6534 Oct 19, 2019
@@ -11,6 +11,10 @@ use std::cell::Cell;
use std::ptr;
use std::sync::atomic::{AtomicBool, Ordering, ATOMIC_BOOL_INIT};

extern "C" {

This comment has been minimized.

Copy link
@sdroege

sdroege Oct 19, 2019

Member
Suggested change
extern "C" {
#[cfg(target_os = "macos")]
extern "C" {

This comment has been minimized.

Copy link
@nipunn1313

nipunn1313 Oct 20, 2019

Author Contributor

I don't think this will compile as is - because cfg! cannot determine at compile time. However, I might be able to use a compile time inline #[cfg]. If we need an if/else chain, we can use something like https://github.com/alexcrichton/cfg-if - but I think in this case we don't need that and it'll be simpler to put some curly braces around the block! Updating momentarily!

@sdroege

This comment has been minimized.

Copy link
Member

sdroege commented Oct 20, 2019

@GuillaumeGomez

This comment has been minimized.

Copy link
Member

GuillaumeGomez commented Oct 20, 2019

Thanks!

@GuillaumeGomez GuillaumeGomez merged commit e36dce5 into gtk-rs:master Oct 20, 2019
1 of 2 checks passed
1 of 2 checks passed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@GuillaumeGomez GuillaumeGomez mentioned this pull request Oct 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.