GNOME Bugzilla – Bug 657567
GTlsInteraction interaction method invocation guarantees
Last modified: 2011-08-30 16:37:18 UTC
I've been working on bug #656343 which involves doing the handshake in a separate thread from the main loop. Part of this problem is that we need to invoke the various GTlsInteraction virtual methods (like ask_password) from this other thread. In order to do this for the current and future GTlsBackends we need some guarantees about the behavior of these virtual methods. * The sync methods should be callable from any thread whether a main loop is currently running or not, and whether that main loop is running in another thread. * Async methods obviously need a main loop running. But these methods should be callable whether the main loop is running in a different thread or not. I implemented this behavior in the GtkTlsInteraction example in gtls-frob [1]. But it's pretty hard to get right, and a tall order to ask all derived classes for GTlsInteraction to do perfectly. So I've implemented these guarantees and behavior GTlsInteraction itself. GTlsInteraction now has a property which is the GMainContext that the virtual interaction methods should run in. By default this is g_main_context_default(). The virtual interaction methods are always guaranteed to be called in this context if the context is 'running', no matter where they're called from. In addition, it's kind of a bummer for derived classes of GTlsInteraction to have to provide both async and sync functions for each interaction type. So in GTlsInteraction I've implemented wrappers which will turn allow an async implementation to be called synchronously, and vice versa. This is only done if a matching async/sync one does not exist. As you can imagine, this is somewhat complex. But I've done good testing for this. I believe having all this code in GTlsInteraction is beneficial and outweighs any downsides, for the following reasons: * There will be multiple GTlsBackend implementations. Each of those should be able to just use GTlsInteraction without worrying about how which thread to call the various interaction methods, and implementing the finicky closures over and over again. * There will be multiple GTlsInteraction derived classes. Having all this code in GTlsInteraction allows each of these guys to be really simple. Implementations can choose between showing modal dialogs (ask_password) or modeless dialogs (ask_password_async/ask_password_finish). In all this code 'solves' about half of the issues of having the handshake in a thread rather than in the main loop.
Created attachment 194980 [details] [review] Add a boxed type for a GMainContext * So that it can be used in properties, such as GTlsInteraction:main_context
Created attachment 194981 [details] [review] gio: Add GTlsInteraction interaction method invocation guarantees * Interaction methods can be called from any thread. * Sync methods can be called whether main loop is running or not. * Derived classes can choose to implement only sync or async interaction method, and GTlsInteraction will fill in the blanks. * Documentation for the above. * Tests for the above.
Branch here: http://cgit.collabora.com/git/user/stefw/glib.git/log/?h=invoke-interaction
Note for later: I need to add g_main_context_get_type to gio.symbols.
(In reply to comment #0) > So I've implemented these guarantees and behavior GTlsInteraction itself. > GTlsInteraction now has a property which is the GMainContext that the virtual > interaction methods should run in. By default this is g_main_context_default(). The standard in gio is that you never pass GMainContexts around directly, but the caller is expected to have used g_main_context_push_thread_default(), and then async operations should complete in whatever context was g_main_context_get_thread_default() when they were started. (For standard async ops, GSimpleAsyncResult handles most of this.) Likewise, objects that asynchronously emit signals or call callbacks should do so in whatever context was thread-default when they were created. There may be some argument that GTlsInteraction is different, since the whole point is to "interact", so you almost always want it to act in g_main_context_default()... not sure. (I haven't looked at the patch yet.)
(In reply to comment #0) > * Async methods obviously need a main loop running. But these methods > should be callable whether the main loop is running in a different thread > or not. This is also not supported in gio, because of the added complexity of having to deal with operations finishing and invoking their callbacks before the "start" method even returns. In fact, it's not even possible to try, because g_main_context_push_thread_default() won't let you push a context that is currently running in another thread, so there's no way to ask an async op to complete in another thread.
Created attachment 195059 [details] [review] Updated for Dan's comments. gio: Add GTlsInteraction interaction method invocation guarantees * Add 'invoke' style method, which can be used to call an interaction from any thread. The interaction will be run in the appropriate #GMainContext * Sync methods can be called whether main loop is running or not. * Derived classes can choose to implement only sync or async interaction method, and the invoke method will fill in the blanks. * Documentation for the above. * Tests for the above.
(In reply to comment #5) > There may be some argument that GTlsInteraction is different, since the whole > point is to "interact", so you almost always want it to act in > g_main_context_default()... not sure. I was sort of thinking that an GTlsInteraction would be 'bound' to a certain main context, and all interaction take place there. But I'm not sure of the exact use cases where this would be desired. So I've taken this functionality out for now (the main-context property). We can always add it or something like it back later if necessary. (In reply to comment #6) > This is also not supported in gio, because of the added complexity of having to > deal with operations finishing and invoking their callbacks before the "start" > method even returns. In fact, it's not even possible to try, because > g_main_context_push_thread_default() won't let you push a context that is > currently running in another thread, so there's no way to ask an async op to > complete in another thread. True, that is a bunch of complexity. And it turns out all the GTlsBackends (than we can imagine today) will only call the various interaction methods synchronously anyway. Since async invocation of GTlsInteraction won't much if at all, I've changed things around and simplified them: * Added g_tls_interaction_invoke_ask_password(). This function makes the guarantees about invoking the virtual methods in the right context, and wrapping async implementations so they're callable synchronously (from another thread). * The g_tls_interaction_ask_password() g_tls_interaction_ask_password_async() and g_tls_interaction_ask_password_finish() go back to normal boring methods that call right through to the virtual methods. These make no guarantees, and are thin wrappers. The only 'extra' thing that they do is return G_TLS_INTERACTION_UNHANDLED if a virtual method is NULL. Pushed to the branch, and attached new patch here.
Comment on attachment 195059 [details] [review] Updated for Dan's comments. >+ * the application's default main loop. A different #GMainContext can be >+ * specified using the GTlsInteraction:main-context: property. not true any more >+ context = g_main_context_get_thread_default (); not quite right; you want to use the context from the thread that the GTlsInteraction was created in, not the context that is the thread-default in the thread where the handshake is occurring (which will probably always be NULL, since the handshake will be occurring in a thread pool's thread). >+ if (context == NULL) >+ context = g_main_context_default (); don't need to do that. all methods that take a GMainContext treat NULL as an alias for g_main_context_default(). >+ * Handle the case where we've been called from within the main context >+ * or in the case where the main context is not running. This approximates >+ * the behavior of a modal dialog. You really don't ever want to hit this case though... are g_tls_interaction_ask_password* even needed any more given invoke_ask_password()?
Created attachment 195180 [details] [review] Thanks for looking it over again. Updated. (In reply to comment #9) > (From update of attachment 195059 [details] [review]) > >+ * the application's default main loop. A different #GMainContext can be > >+ * specified using the GTlsInteraction:main-context: property. > > not true any more Removed. > >+ context = g_main_context_get_thread_default (); > > not quite right; you want to use the context from the thread that the > GTlsInteraction was created in, not the context that is the thread-default in > the thread where the handshake is occurring (which will probably always be > NULL, since the handshake will be occurring in a thread pool's thread). I agree that's better. Changed. > >+ if (context == NULL) > >+ context = g_main_context_default (); > > don't need to do that. all methods that take a GMainContext treat NULL as an > alias for g_main_context_default(). Done. > >+ * Handle the case where we've been called from within the main context > >+ * or in the case where the main context is not running. This approximates > >+ * the behavior of a modal dialog. > > You really don't ever want to hit this case though... We hit this case in socket-client and any command line application. It's certainly not rare. > are g_tls_interaction_ask_password* even needed any more given > invoke_ask_password()? They won't be used by the current GTlsBackends right now, but they might be used by future code. For example code that decrypts encrypted keys when loading them, etc. I think it's worth having these straight up wrappers of the virtual methods.
Comment on attachment 195180 [details] [review] Thanks for looking it over again. Updated. >+ * interaction methods to show the required dialogs. Such a derived class >+ * is #GtkTlsInteraction. may want to leave that bit out of the docs for now... if you don't already have GtkTlsInteraction done, it may be too late to get it in for 3.2 otherwise looks good
Alright, took that out. And fixed gio.symbols, and a race condition in the tests. Merged.