gtk::idle_add() and gtk::timeout_add() are unsafe

[sdroege]

Different than the ones in GLib, these do not require Send on the closure. It is however possible to call them from an arbitrary thread, in which case this would allow moving something that is not Send from that arbitrary thread to the GTK main thread.

Of course by requiring Send on the closure, this becomes rather less useful because it's usually used for doing things on GTK objects from the main thread... and no GTK object is Send so couldn't be used anymore.

[sdroege]

Something like the following could be used to work around the limitations if we add Send:

use std::thread;
use std::ops::Deref;
#[derive(Clone, Debug)]
struct SendCell<T>(T, thread::ThreadId);

impl<T> SendCell<T> {
    pub fn new(data: T) -> Self {
        SendCell(data, thread::current().id())
    }
}

impl<T> Deref for SendCell<T> {
    type Target = T;

    fn deref(&self) -> &T {
        assert_eq!(thread::current().id(), self.1);
        &self.0
    }
}

unsafe impl<T> Send for SendCell<T> {}

[sdroege]

Or the more complete version here, including a case where it's needed: https://github.com/sdroege/gstreamer-rs/blob/5676aeb3efaabe263f2c524746032c8441671830/examples/src/bin/gtksink.rs#L126

I would suggest that we get this into glib for now... it seems like something that is needed commonly enough. And then fix up the functions mentioned here.

@GuillaumeGomez @EPashkin Any comments?

[sdroege]

As mentioned here https://twitter.com/simukis/status/893498864279355392 we could also add idle_add_on_this_thread or similar, but that has the disadvantage that you need to know that this thread currently owns the default main context (or that it owns the main context you care about). And doing it wrong is even harder to debug then.

[nagisa]

The wrapper over gtk_idle_add/gtk_timeout_add could check if no cross-thread sending happens.

Something like this:

fn idle_add<F>(f: F) {
    let thread_default = g_main_context_get_thread_default();
    if is_default_ctx_owned_by_this_thread {
        panic!("no can do!");
    }
    gtk_idle_add(...);
}

If unstable features are available, check could be replaced with, which would just pass the Send stuff through:

trait Foo {
    fn assert_sendness() {}
}

impl<T> Foo for T { 
    default fn assert_sendness() { if ... { panic! () }  }
} 
impl <T: Send> Foo for T {
    fn assert_sendness() {}
}

<F as Foo>::assert_sendness()

But yeah, I’m not sure how best to detect if the default context is owned by the current thread. g_main_context_is_owner() would probably be the important function, but I think comparing result of g_main_context_get_thread_default() with g_main_context_default() could also work.

[sdroege]

It would also mean that we need two variants for each GSource thingy (GStreamer bus watch, idle_add(), timeout_add(), timeout_add_seconds(), the POSIX signal watch, the child watch, ...). One for "this thread", one for "that other thread that runs the current thread default main context".
You still want to be able to use all these functions from other threads with the Send requirement.

There's also the problem that in theory you can run the same main context on multiple threads. Nothing is preventing that, as long as it doesn't happen at the same time, and it's not necessarily a problem (except for GTK on Windows).

[EPashkin]

@sdroege, you think that this macro is not enough to safety https://github.com/gtk-rs/gtk/blob/master/src/rt.rs#L21-L22?

[sdroege]

Ah I missed that part. It is indeed enough for the function variants in gtk, as long as the default main context is only ever handled from a single thread (which might be safe to assume for gtk applications). It however makes these functions a bit useless (you can't use them from another thread, which is often exactly why you want to use them), but for that there are the ones in glib still.

The more general problem for all the other things (in e.g. GLib and GStreamer) still stays though.

[sdroege]

I guess it makes sense closing this one then and moving everything to GLib instead.

[EPashkin]

👍 for adding SendCell from gstreamer in GLib.

[EPashkin]

Maybe without From implementation to prevent runtime errors with closures.