GNOME Bugzilla – Bug 742599
Clean up GUnixMountMonitor
Last modified: 2015-03-02 20:16:36 UTC
This class is currently problematic: it is a global singleton which fires signals into the thread-default main context that was in effect for the first person to create it. Let's turn it into a per-thread singleton, similar to GAppInfoMonitor. It's not surprising that we'd share a lot of code between these two cases, so let's create a helper API for that and use it from both. The helper API takes a different approach from the code that was originally used inside of the app info monitor and I think it's a lot easier to understand. Finally, we hang all of this on top of a new API designed to enforce correct access to objects: g_main_context_ensure_is_owner()
Created attachment 294106 [details] [review] GThread: add internal "debug name" helper Add an internal function to get the name of a thread for display in debugging messages. We support reporting "the main thread" for the thread that our ctor got run in (which is almost definitely what most people would consider to be 'the main thread').
Created attachment 294107 [details] [review] Add g_main_context_ensure_is_owner() This new function is intended to be used for verifying that operations on objects that belong to a given main context are only made from valid places. The usual rule is that the operations must be performed while the main context is acquired, but an exception is made for the global default main context so that things can be set up before running the main loop.
Created attachment 294108 [details] [review] Add internal helper GContextSpecificGroup Add a new internal helper called GContextSpecificGroup. This is a mechanism for helping to maintain a group of context-specific monitor objects (eg: GAppInfoMonitor, GUnixMountMonitor).
Created attachment 294109 [details] [review] GAppInfoMonitor: port to GContextSpecificGroup
Created attachment 294110 [details] [review] Rename g_unix_mount_monitor_new() to _get() This is a singleton, but we have a function called _new() to get it. What's worse is that the documentation makes no mention of this, and actually specifically says that a new monitor will be created each time.
Created attachment 294111 [details] [review] Deprecate g_unix_mount_monitor_set_rate_limit() Deprecate g_unix_mount_monitor_set_rate_limit() and turn it into a no-op. This function doesn't behave as advertised. It only controls rate limiting for filesystem-based monitors. It has no impact over reporting mount changes on Linux, for example, because those are based on polling for changes in /proc (which doesn't use filesystem monitors). It also has no impact on Mac OS because a library interface is used there. This was added in https://bugzilla.gnome.org/show_bug.cgi?id=521946 in order to be used by HAL, which is effectively dead. udisks no longer uses this code at all.
Created attachment 294112 [details] [review] gunixmounts.c: add fold markers This is a large file with a lot of very complicated code in it. Add some fold markers to make things a bit more manageable.
Created attachment 294113 [details] [review] gunixmounts: move GUnixMountMonitor code Move this code to the correct part of the file. While we're at it, drop an unused #define MOUNT_POLL_INTERVAL.
Created attachment 294114 [details] [review] Make GUnixMountMonitor per-context GUnixMountMonitor was not threadsafe before. It was a global singleton which emitted signals in the first thread that happened to construct it. Move it to a per-context singleton model where each GMainContext gets its own GUnixMountMonitor. Monitor for the changes from the GLib worker thread and dispatch the results to each context with an active monitor.
Review of attachment 294106 [details] [review]: ::: glib/gthread.c @@ +991,3 @@ + + else + return "unnamed thread"; how about snprintf (buffer, sizeof (buffer), "unnamed thread %d", gettid ()) ?
gettid() is not available. See https://sourceware.org/bugzilla/show_bug.cgi?id=6399 (longstanding bug). I wanted to at least be able to identify the main thread here, but calling g_thread_self() from glib_init (to get a reasonable definition of "the main thread") means that we initialise g_slice even earlier, causing test case failures.... ugh.
Review of attachment 294110 [details] [review]: ::: gio/gunixmounts.c @@ +1523,3 @@ + * entries). + * + * Returns: (transfer full): the #GUnixMountMonitor. Aren't our singleton get() functions typically transfer none ? gtk_settings_get_default() gdk_display_get_default() gdk_screen_get_default() ... @@ +1549,3 @@ + * Returns: a #GUnixMountMonitor. + * + * Deprecated:2.44:Use g_unix_mount_monitor_get() instead. I think we typically leave a space after Deprecated:2.44:
Review of attachment 294107 [details] [review]: ::: glib/gmain.c @@ +4595,3 @@ + LOCK_CONTEXT (context); + + is_owner = context->owner == G_THREAD_SELF; can use "self" here.
Review of attachment 294110 [details] [review]: ::: gio/gunixmounts.c @@ +1523,3 @@ + * entries). + * + * Returns: (transfer full): the #GUnixMountMonitor. Agree. We can take a ref on it inside the _new() helper.
Review of attachment 294109 [details] [review]: ::: gio/gappinfo.c @@ +1126,3 @@ + G_TYPE_APP_INFO_MONITOR, + G_STRUCT_OFFSET (GAppInfoMonitor, context), + NULL); This doesn't look right. This just returns the singleton, however, the old code returned an owned ref to the singleton. i.e. the first ref to the singleton if it was created, but an extra ref otherwise.
Review of attachment 294114 [details] [review]: Looks good to me in general other than the refcount issues. ::: gio/gunixmounts.c @@ +1562,3 @@ + G_TYPE_UNIX_MOUNT_MONITOR, + G_STRUCT_OFFSET(GUnixMountMonitor, context), + mount_monitor_start); Same issue here. This doesn't look like a transfer full.
Otherwise this looks good to me.
Comment on attachment 294106 [details] [review] GThread: add internal "debug name" helper >Add an internal function to get the name of a thread for display in >debugging messages. We support reporting "the main thread" for the >thread that our ctor got run in (which is almost definitely what most >people would consider to be 'the main thread'). there's nothing about "the main thread" in that patch...
Comment on attachment 294107 [details] [review] Add g_main_context_ensure_is_owner() >+ * This means that calling this function, at any time, on the unacquired >+ * main context is incompatible with acquiring the main context in two >+ * different threads, but probably you didn't want to do that anyway. So... that means that one library could acquire the default GMainContext from different threads at different times, but never call g_main_context_ensure_is_owner(), and that library would be correct; and a second library could call g_main_context_ensure_is_owner() from the main thread, but never acquire the default main context from any other thread, and that library would be correct; but if you link both libraries into the same program, then suddenly one or both of them contains a programmer error?
(In reply to comment #18) > there's nothing about "the main thread" in that patch... I'll adjust the message. I took that feature out because it was based on a ctor calling G_THREAD_SELF, which was causing trouble with too-early-init of the slice allocator.... We could also just move GThread to being passed on malloc()... (In reply to comment #19) > So... that means that one library could acquire the default GMainContext from > different threads at different times, but never call > g_main_context_ensure_is_owner(), and that library would be correct; and a > second library could call g_main_context_ensure_is_owner() from the main > thread, but never acquire the default main context from any other thread, and > that library would be correct; but if you link both libraries into the same > program, then suddenly one or both of them contains a programmer error? It's my opinion that it's already a case that there is a programmer error here. That's like saying "one library could validly only ever access a shared resource from thread 1" and "another library could validly only ever access a shared resource from thread 2" and, of course, when you put them in the same program you now have a bug. The main idea here is that if the main context is acquired, it's like holding a lock. Because the main context isn't acquired during main() after people call gtk_init() but before the mainloop is running, we need to act as if it is acquired. Now the problem becomes a problem about assuring that we still effectively have mutual exclusion. The only way we can do that is if the main thread is indeed the only thread that the global default main context is ever run in. This is the 99.99% case. If we let two separate threads run unacquired code then there is a chance they could stomp on each other. I did think of a problem with this whole setup after the fact, though -- it assumes that there is not some other means of coordinating the mutual exclusion, like the GDK lock. For this reason, I think we can really only enable this stuff with G_DEBUG=thread-debug or so... We could also try to add support to the GDK lock for reporting itself down to gmain.c so that we can deal with that case....
Comment on attachment 294108 [details] [review] Add internal helper GContextSpecificGroup needs docs >+ context = g_main_context_get_thread_default (); >+ if (!context) >+ context = g_main_context_default (); FTR, note that g_main_context_REF_thread_default() does that for you (although maybe there's some reason you don't want a ref on it here?) >+ G_STRUCT_MEMBER (GMainContext *, instance, context_offset) = context; It would be less hacky to just require the type to have a G_TYPE_MAIN_CONTEXT property that GContextSpecificGroup could set. (Especially if there's any chance the API might be used on non-internal objects where you don't want to have a public field?)
(In reply to comment #21) > FTR, note that g_main_context_REF_thread_default() does that for you (although > maybe there's some reason you don't want a ref on it here?) I indeed don't want the ref. In the case that it's just a lookup of an existing entry, we'd have to unref it later. In the case that we create a new one... I'm not sure. Since we will have a function which can return the main context via an interface call, probably we do want to hold an actual ref -- but we should take the ref only in that case. > > >+ G_STRUCT_MEMBER (GMainContext *, instance, context_offset) = context; > > It would be less hacky to just require the type to have a G_TYPE_MAIN_CONTEXT > property that GContextSpecificGroup could set. (Especially if there's any > chance the API might be used on non-internal objects where you don't want to > have a public field?) I did this specifically because G_PRIVATE_OFFSET is a thing.
Review of attachment 294110 [details] [review]: ::: gio/gunixmounts.c @@ +1523,3 @@ + * entries). + * + * Returns: (transfer full): the #GUnixMountMonitor. In this case I think we want _get() to return a ref. As a counterexample, see GAppInfoMonitor. The reason that we want it here is the same as the reason that we want it there: when the user stops watching, we can tear it down. Unlike in Gtk, this object does not always need to exist (and because it's a monitor, there is a large external cost to its existence, in terms of extra wakeups).
Created attachment 295488 [details] [review] Add internal helper GContextSpecificGroup Add a new internal helper called GContextSpecificGroup. This is a mechanism for helping to maintain a group of context-specific monitor objects (eg: GAppInfoMonitor, GUnixMountMonitor).
Created attachment 295489 [details] [review] GAppInfoMonitor: port to GContextSpecificGroup
Created attachment 295490 [details] [review] Rename g_unix_mount_monitor_new() to _get() This is a singleton, but we have a function called _new() to get it. What's worse is that the documentation makes no mention of this, and actually specifically says that a new monitor will be created each time.
Created attachment 295491 [details] [review] Deprecate g_unix_mount_monitor_set_rate_limit() Deprecate g_unix_mount_monitor_set_rate_limit() and turn it into a no-op. This function doesn't behave as advertised. It only controls rate limiting for filesystem-based monitors. It has no impact over reporting mount changes on Linux, for example, because those are based on polling for changes in /proc (which doesn't use filesystem monitors). It also has no impact on Mac OS because a library interface is used there. This was added in https://bugzilla.gnome.org/show_bug.cgi?id=521946 in order to be used by HAL, which is effectively dead. udisks no longer uses this code at all.
Created attachment 295492 [details] [review] gunixmounts.c: add fold markers This is a large file with a lot of very complicated code in it. Add some fold markers to make things a bit more manageable.
Created attachment 295493 [details] [review] gunixmounts: move GUnixMountMonitor code Move this code to the correct part of the file. While we're at it, drop an unused #define MOUNT_POLL_INTERVAL.
Created attachment 295494 [details] [review] Make GUnixMountMonitor per-context GUnixMountMonitor was not threadsafe before. It was a global singleton which emitted signals in the first thread that happened to construct it. Move it to a per-context singleton model where each GMainContext gets its own GUnixMountMonitor. Monitor for the changes from the GLib worker thread and dispatch the results to each context with an active monitor.
Created attachment 295495 [details] [review] tests: add some tests for GContextSpecificGroup
Review of attachment 294106 [details] [review]: punting on this for now
Review of attachment 294107 [details] [review]: ditto
New patches are at wip/desrt/mountmonitor. I think it's more or less ready to push now.
Attachment 295488 [details] pushed as c90b083 - Add internal helper GContextSpecificGroup Attachment 295489 [details] pushed as 7202745 - GAppInfoMonitor: port to GContextSpecificGroup Attachment 295490 [details] pushed as 73d4e6f - Rename g_unix_mount_monitor_new() to _get() Attachment 295491 [details] pushed as 3167c61 - Deprecate g_unix_mount_monitor_set_rate_limit() Attachment 295492 [details] pushed as f535218 - gunixmounts.c: add fold markers Attachment 295493 [details] pushed as ae38d2b - gunixmounts: move GUnixMountMonitor code Attachment 295494 [details] pushed as 548c165 - Make GUnixMountMonitor per-context Attachment 295495 [details] pushed as 88745c2 - tests: add some tests for GContextSpecificGroup I think we've gone enough times around on this...