GNOME Bugzilla – Bug 659507
main context is not pushed when creating dbus proxy in one case leading to signals on the wrong thread
Last modified: 2021-05-17 16:28:59 UTC
Created attachment 196992 [details] [review] Push the thread default context before creating a new bus so the signals go on the right thread In one case, the main context isn't set properly set on the thread before creating the bus leading the signals to arrive on the main() thread... Attached patch should fix it.
Created attachment 196998 [details] [review] Push the thread default context before creating a new bus so the signals go on the right thread. Oops, forgot to remove one line
0.18 always uses g_main_context_get_thread_default () now.
Review of attachment 196998 [details] [review]: ::: libgupnp/gupnp-network-manager.c @@ -764,3 +766,4 @@ dbus_proxy = g_dbus_proxy_new_for_bus_sync ( G_BUS_TYPE_SYSTEM, - G_DBUS_PROXY_FLAGS_DO_NOT_LOAD_PROPERTIES, + G_DBUS_PROXY_FLAGS_DO_NOT_LOAD_PROPERTIES | + G_DBUS_PROXY_FLAGS_DO_NOT_CONNECT_SIGNALS, This change is not related to the main-context changes - what's that for?
I was still following gitorious, forgot to switch over to git.gnome.org I see that in create_loopback_context(), you forget to set the right default main context before creating the gupnp context.. Because unless it's a callback from a GSimpleAsyncResult, you must push thread-local context correctly before doing any async call in an async callback. See bug #646957 for the gory details. You probably want to triple checks other idle calls elsewhere. A good test for that is to run the test suite on a non-default main context (ie not having the default one running at all). Also either remove the old API and change the soname or make it do push/pop around calling the new API. Otherwise it will break all the existing gupnp-igd users. The properties thing is just to make sure that no signal callbacks happen in the main thread. So it's kind of related. Always, it saves a couple dbus calls.
(In reply to comment #4) > I was still following gitorious, forgot to switch over to git.gnome.org > > I see that in create_loopback_context(), you forget to set the right default > main context before creating the gupnp context.. Because unless it's a callback > from a GSimpleAsyncResult, you must push thread-local context correctly before > doing any async call in an async callback. See bug #646957 for the gory > details. You probably want to triple checks other idle calls elsewhere. So despite the idle source being hooked on the g_main_context_get_thread_default() we'd need to push/pop around the g_signal_emit_by_name ? > Also either remove the old API and change the soname or make it do push/pop The soname has changed. > around calling the new API. Otherwise it will break all the existing gupnp-igd > users. > > The properties thing is just to make sure that no signal callbacks happen in > the main thread. So it's kind of related. Always, it saves a couple dbus calls. Ok.
Created attachment 197019 [details] context manager in thread That's the output of attached program with network manager context manager: ** (process:12362): DEBUG: Main Thread: 0x7f508a315800 ** (process:12362): DEBUG: Worker Thread: 0x7f50873e8700 ** (process:12362): DEBUG: Thread: 0x7f50873e8700 ** (process:12362): DEBUG: ====> New context for interface lo ** (process:12362): DEBUG: Thread: 0x7f50873e8700 ** (process:12362): DEBUG: ====> New context for interface eth0 ** (process:12362): DEBUG: Thread: 0x7f50873e8700 ** (process:12362): DEBUG: ====> New context for interface usb0 ** (process:12362): DEBUG: Thread: 0x7f50873e8700 ** (process:12362): DEBUG: ====> New context for interface wlan0 ** (process:12362): DEBUG: Thread: 0x7f50873e8700 ** (process:12362): DEBUG: ====> Context for interface wlan0 gone
(In reply to comment #5) > (In reply to comment #4) > > I was still following gitorious, forgot to switch over to git.gnome.org > > > > I see that in create_loopback_context(), you forget to set the right default > > main context before creating the gupnp context.. Because unless it's a callback > > from a GSimpleAsyncResult, you must push thread-local context correctly before > > doing any async call in an async callback. See bug #646957 for the gory > > details. You probably want to triple checks other idle calls elsewhere. > > So despite the idle source being hooked on the > g_main_context_get_thread_default() we'd need to push/pop around the > g_signal_emit_by_name ? Yes... gmain never pushes it's own main context on the thread local main context stack. GSimpleAsyncResult does it, but any other callback system needs to do it.. One should also verify that libsoup does it correctly..
Actually, I see zeeshan did change the soname around june 6. But since it's not source compatible, please also rename the .pc file and the .gir files.. Can you also stop breaking the API every 6 months ?
(In reply to comment #7) > > So despite the idle source being hooked on the > > g_main_context_get_thread_default() we'd need to push/pop around the > > g_signal_emit_by_name ? > > Yes... gmain never pushes it's own main context on the thread local main > context stack. GSimpleAsyncResult does it, but any other callback system needs > to do it.. One should also verify that libsoup does it correctly.. Sorry, but - then the new context signal for lo should have ended up in main thread - which it clearly doesn't. And isn't that a huge work-around for people that fail to push the main-context of their worker thread correctly?
(In reply to comment #8) > Actually, I see zeeshan did change the soname around june 6. But since it's not > source compatible, please also rename the .pc file and the .gir files.. > > Can you also stop breaking the API every 6 months ? AFAIK, we follow the proper deprecation process.
@zeeshan: deprecation means you keep on fully supporting the old API, not that you make it half-functional so that it still compiles but crashes randomly when you're not lucky. @jens: well, I didn't design gio, but it seems that you're not supposed to set it on the thread, but every time you try to use a function..
(In reply to comment #11) > @zeeshan: deprecation means you keep on fully supporting the old API, not that > you make it half-functional so that it still compiles but crashes randomly when > you're not lucky. True but as Jens pointed out, you are asking for a rather huge work-around for an issue thats probably only going to hit your use-case only (I don't know of anyone who uses gupnp from different threads) and only if you don't push your context. > @jens: well, I didn't design gio, but it seems that you're not supposed to set > it on the thread, but every time you try to use a function.. References?
Actually, that's how farsight2 and libnice use gupnp.. from a dedicated gupnp thread. So that's why I'm worried.. about the GIO, I'm not sure if it's documented anywhere, I was going to write a blog post about it.
To do it properly in the new GIO style, I believe that gupnp_service_proxy_begin_action() should return it's reply in the context that's on top of the stack when the _begin_action() is called, not the context that was on top when the proxy was created.. or when the gupnpcontext was created? This means you need to save the current context in the _begin_* call and possibly (probably?) call the callback in that stored context.. Using GSimpleAsyncResult will probably help implement that correctly.
(In reply to comment #14) > To do it properly in the new GIO style, I believe that > gupnp_service_proxy_begin_action() should return it's reply in the context > that's on top of the stack when the _begin_action() is called, not the context > that was on top when the proxy was created.. or when the gupnpcontext was > created? This means you need to save the current context in the _begin_* call > and possibly (probably?) call the callback in that stored context.. Using > GSimpleAsyncResult will probably help implement that correctly. Woah, wait. That means that: a) You either arbitrarily switch the GMainContext of a thread b) Call functions from a different thread! with a different context than the one the proxy lives in. Both seems entirely wrong.
This is exactly how the GIO model works. GMainContexts aren't tied to a thread.. They're tied to one or more gmainloops (which can run in any thread one at a time) and then you call async methods and expect the reply to come from the thread that this GMainContext is running in.. I see you haven't really understood the gio model, but many people don't as it's quite terribly documented. I should do a series of post on planet gnome about it.
There are two things here: a) We trying to support the possibility to run all GUPnP action in a separate worker thread (useful for e.g. non-glib-mainloop systems (Qt, Windows) or people who don't know better (Qt)) with a separate dedicated context and ensure that all events end up in that thread. This kind of works with 0.16 modulo some bugs where the context push is missing (as the initial report shows) and problems by users to understand what to do there and I'm quite confident that it works with 0.18. There was and is no guarantee that you can call functions from separate threads and it's supposed to work. What we've definitely lost is the possibility to use a GMainContext which isn't either default nor thread-default, that's true and I also see the issue that arises here, horridly mixed contexts and crashes of all kind if people use push/pop around the context manager creation. Still I'm a bit lost trying to find the the use-case for that. b) you trying to coerce the GIO model of being able to change the target context for every function call. This wasn't supported in 0.16 and AFAIK never intended.
(In reply to comment #16) > This is exactly how the GIO model works. GMainContexts aren't tied to a > thread.. They're tied to one or more gmainloops (which can run in any thread > one at a time) and then you call async methods and expect the reply to come > from the thread that this GMainContext is running in.. > > I see you haven't really understood the gio model, but many people don't as > it's quite terribly documented. I should do a series of post on planet gnome > about it. Here is what the docs for g_main_context_push_thread_default says (words worth putting stress on, are highlighted): "*Normally* you would call this function *shortly after creating a new thread*, passing it a GMainContext which will be run by a GMainLoop in that thread, to set a new default context for *all async operations in that thread*. (In this case, you don't need to ever call g_main_context_pop_thread_default().) In *some cases* however, you may want to schedule a single operation in a non-default context, or temporarily use a non-default context in the main thread. *In that case*, you can wrap the call to the asynchronous operation inside a g_main_context_push_thread_default() / g_main_context_pop_thread_default() pair, but it is up to you to ensure that no other asynchronous operations accidentally get started while the non-default context is active. Beware that libraries that predate this function may not correctly handle being used from a thread with a thread-default context. Eg, see g_file_supports_thread_contexts()." So from these docs alone, I see that our approach is very much compatible with that of GIO maintainers/developers. A blog post *from you* will not prove us wrong, however GIO maintainers/developers saying otherwise, will!
(In reply to comment #8) > Actually, I see zeeshan did change the soname around june 6. But since it's not > source compatible, please also rename the .pc file and the .gir files.. You do have a very valid and important point there. We should really have proper versioning on .pc and .gir files. Thing is that we thought we would have stable APIs long time ago, alas we were naive.
gupnp-igd's SimpleIgdThread object does exactly model 1 .. that is, it starts a new thread with its own main context and runs everything in there.. and pushes operations there by adding idle calls. It is actually used by non-GLib apps like amsn (which is in TCL so uses the TCL/Tk main loop). That said, other GIO based systems, like GDBus and the GIOStream stuff allow you to do push()/async()/pop() so that's what people expect. If you decide to not support this, I'm fine with that, but it should be documented very clearly.
Created attachment 197383 [details] [review] Revert "Deprecate main_context property." This reverts commit 541f3752f89899c6ecd775abcfbd89afbb6fc403. Conflicts: libgssdp/gssdp-client.c
Created attachment 197384 [details] [review] Add gupnp_main_context_create_full function The new function takes a GMainContext again. Change gupnp_context_manager_create to be a wrapper to gupnp_main_context_manager_create_full (port, g_main_context_get_thread_default ())
Created attachment 197385 [details] [review] Revert "Drop main-context property." This reverts commit 831395e0c8b295d4bb24aa96d68012f6e3ecb7cc. Conflicts: libgupnp/gupnp-context-manager.c Conflicts: libgupnp/gupnp-context-manager.c libgupnp/gupnp-unix-context-manager.c
(In reply to comment #20) > > That said, other GIO based systems, like GDBus and the GIOStream stuff allow > you to do push()/async()/pop() so that's what people expect. If you decide to > not support this, I'm fine with that, but it should be documented very clearly. Yeah, we should support that. However, there are certain things that aren't exactly simple async calls, like gupnp context has to create some sockets and assign them to a context and ContextManager has to make some calls to NetworkManager behind the scene. Wonder how do deal with that?
(In reply to comment #24) > (In reply to comment #20) > > > > That said, other GIO based systems, like GDBus and the GIOStream stuff allow > > you to do push()/async()/pop() so that's what people expect. If you decide to > > not support this, I'm fine with that, but it should be documented very clearly. > > Yeah, we should support that. However, there are certain things that aren't > exactly simple async calls, like gupnp context has to create some sockets and > assign them to a context and ContextManager has to make some calls to > NetworkManager behind the scene. Wonder how do deal with that? There's also libsoup that takes the context on creation. You could always inject the tesulting event with a idle source, of course. But that's a lot of book-keeping or - alternatively - move to gio-style function calls with GAsyncResult.
Review of attachment 197384 [details] [review]: I'm not sure how gupnp_main_context_manager_create_full() is different from the original gupnp_main_context_manager_new() ?
Let's commit the patches from this bug as a first step.. Then you can try running the unit tests in gupnp-igd, make sure they still work, then make a release and lets push that into the distros.. Because right now, gupnp is entirely broken in empathy,pidgin,etc in Fedora 16 for example.
On top of your patches: --- a/libgupnp/gupnp-simple-context-manager.c +++ b/libgupnp/gupnp-simple-context-manager.c @@ -74,7 +74,7 @@ create_and_signal_context (const char *interface, context = g_initable_new (GUPNP_TYPE_CONTEXT, NULL, &error, - "main-context", interface, + "main-context", main_context, "interface", interface, "port", port, NULL);
(In reply to comment #26) > Review of attachment 197384 [details] [review]: > > I'm not sure how gupnp_main_context_manager_create_full() is different from the > original gupnp_main_context_manager_new() ? _new was never a constructor, always a factory method. The name change would be to reflect the behavior, also GUPnPContextManager is an abstract class now. (In reply to comment #27) > Let's commit the patches from this bug as a first step.. Then you can try > running the unit tests in gupnp-igd, make sure they still work, then make a > release and lets push that into the distros.. We won't apply those patches as they are broken, something went wrong while reverting.. _IF_ we would go back it would be the patches which are currently in Harmattan (where I reverted it not because I believe it's broken, VOIP calls worked just fine, but nobody cared due to massive bugzilla FUD spreading). > > Because right now, gupnp is entirely broken in empathy,pidgin,etc in Fedora 16 > for example. Proof? Where are the downstream bugs? Your test-suite doesn't count, it's racy as hell and doesn't even work properly with 0.16.
(In reply to comment #29) > Proof? Where are the downstream bugs? Your test-suite doesn't count, it's racy > as hell and doesn't even work properly with 0.16. Ok, I see the potential issue of using the wrong context with the current threaded IGD and the new gupnp api. It would have been a hell of a lot easier to understand your problem if you would have pointed to your constructor chain-up and where you actually create the context manager.
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/gupnp/-/issues/27.