Comment on attachment 148406 [details] [review]
patch to help reproduce the bug

Setting patch status to reviewed to get it off the review-list.

For the record, an alternative version of the reproducer would be

diff --git a/libjuicer/sj-metadata-getter.c b/libjuicer/sj-metadata-getter.c
index ac700fc..01ea347 100644
--- a/libjuicer/sj-metadata-getter.c
+++ b/libjuicer/sj-metadata-getter.c
@@ -172,7 +172,7 @@ lookup_cd (SjMetadataGetter *mdg)
   GError *error = NULL;
   gboolean found = FALSE;
   GType types[] = {
-#ifdef HAVE_MUSICBRAINZ5
+#ifdef HAVE_MUSICBRAINZ5_FOO
     SJ_TYPE_METADATA_MUSICBRAINZ5,
 #endif /* HAVE_MUSICBRAINZ5 */
     SJ_TYPE_METADATA_GVFS

Created attachment 266980 [details] [review]
client: Remove unused vfs_connection_setup argument

This internal function is being passed an "async" boolean but does not make
use of it.

Created attachment 266981 [details] [review]
client: Invalidate dbus ids of closed dbus connections

GDaemonVfs is keeping a cache
When a DBus connection goes away, it can be removed from this cache with
a _g_daemon_vfs_invalidate_dbus_id() call.
gvfsdaemondbus.c is detecting when DBus connection are closed by connecting
to the GDBusConnection::closed signal, but upon receiving such a signal, it
was not calling _g_daemon_vfs_invalidate_dbus_id() to notify GDaemonVfs
that this connection is no longer available.

When accessing a cdda:// URI, gvfsd-cdda will exit each time the CD is
ejected. Without this fix, when you first access the CD, gvfsdaemondbus.c
will cache the DBus address that was used to talk to gvfsd-cdda in
GDaemonVfs::mount_cache.
Then after ejecting the CD and reinserting it, the DBus address of gvfsd-cdda
will have changed, but GDaemonVfs::mount_cache will contain stale data from
the first cdda access, causing this error to be reported:
"The name :1.257 was not provided by any .service files"

By calling _g_daemon_vfs_invalidate_dbus_id() in the
GDBusConnection::closed signal handler, we ensure the stale entry is
removed from GDaemonVfs::mount_cache.

Created attachment 266982 [details] [review]
client: Notify of DBus connection closure when cleaning up TLS data

gvfsdaemondbus.c is keeping track of local DBus connections by using a GPrivate
variable (local_connections). This data is cleaned up by free_local_connections()
when the thread dies. However, this is only taking care of freeing memory,
and is not notifying the rest of the client code that some DBus connection
were tore down. In particular, GDaemonVfs::mount_cache keeps a cache of
these DBus connections, and _g_daemon_vfs_invalidate_dbus_id() needs to be
called to remove the stale entry. vfs_connection_closed() is doing the
needed work, so we use it to properly mark the DBus connection as closed.

This causes bgo#602855 as sound-juicer is using a short-lived thread to do
its cdda:// IOs. The first time, the DBus connection to gvfsd-cdda is
cached in GDaemonVfs::mount_cache. The corresponding GDBusConnection in
gvfsdaemondbus.c gets disposed of when the sound-juicer thread dies. In
particular, this means GDBusConnection::closed will never be triggered for
that connection. When the CD is ejected, gvfsd-cdda exits, and a new one is
spawned when a new CD is inserted. However, GDaemonVfs::mount_cache still
contains a stale entry for the gvfsd-cdda that was used the first time.

This commit makes sure cached connections are removed from
GDaemonVfs::mount_cache when we remove the thread-local GDBusConnections

Review of attachment 266981 [details] [review]:

Just noticed this log is incomplete, I've changed it to the one below. Note that I'm not sure about the correct vocabulary (DBus ID vs name vs address). I've tried to go with 'name' everywhere in the updated log.

client: Notify of DBus connection closure when cleaning up TLS data

    gvfsdaemondbus.c is keeping track of local DBus connections by using a GPrivate
    variable (local_connections). This data is cleaned up by free_local_connections()
    when the thread dies. However, this is only taking care of freeing memory,
    and is not notifying the rest of the client code that some DBus connection
    were tore down. In particular, GDaemonVfs::mount_cache keeps a cache of
    these DBus connections, and _g_daemon_vfs_invalidate_dbus_id() needs to be
    called to remove the stale entry. vfs_connection_closed() is doing the
    needed work, so we use it to properly mark the DBus connection as closed.

    This causes bgo#602855 as sound-juicer is using a short-lived thread to do
    its cdda:// IOs. The first time, the DBus connection to gvfsd-cdda is
    cached in GDaemonVfs::mount_cache. The corresponding GDBusConnection in
    gvfsdaemondbus.c gets disposed of when the sound-juicer thread dies. In
    particular, this means GDBusConnection::closed will never be triggered for
    that connection. When the CD is ejected, gvfsd-cdda exits, and a new one is
    spawned when a new CD is inserted. However, GDaemonVfs::mount_cache still
    contains a stale entry for the gvfsd-cdda that was used the first time.

    This commit makes sure cached connections are removed from
    GDaemonVfs::mount_cache when we remove the thread-local GDBusConnections

Review of attachment 266981 [details] [review]:

Is this really needed?
Every sync gvfs call starts in some way or another with a call to _g_dbus_connection_get_sync(), which
looks up the connection cache for an existing connection. On a cache hit it then does:

  if (g_dbus_connection_is_closed (connection))
    {
      /* The mount for this connection died, we invalidate
       * the caches, and then caller needs to retry.
       */
      invalidate_local_connection (dbus_id, error);
      return NULL;
     }

It seems to me like this should fix the same problem as this patch tries to solve, but without installing a signal handler which will run in some unknown thread context, which sounds very scary.

::: client/gvfsdaemondbus.c
@@ +498,3 @@
+
+  data = g_object_get_data (G_OBJECT (connection), "connection_data");
+  g_assert (data != NULL);

Its a bit weird to call vfs_connection_setup() which allocates connection_data, and then directly get it just to set the dbus_id. Can't you just add a dbus_id argument to vfs_connection_setup. Seems nicer.

Review of attachment 266982 [details] [review]:

I don't think this is right either. I think what is missing is a call in free_local_connections that calls g_daemon_vfs_invalidate_dbus_id() on all the keys in local->connections.

Review of attachment 266980 [details] [review]:

Actually, i think it *should* use the async argument, and only set up the "closed" callback in that case, because close tracking is done differently in the sync case.

Review of attachment 266981 [details] [review]:

> Is this really needed?
> Every sync gvfs call starts in some way or another with a call to _g_dbus_connection_get_sync(), which
> looks up the connection cache for an existing connection. On a cache hit it then does:
> if (g_dbus_connection_is_closed (connection))
> {
>  /* The mount for this connection died, we invalidate
>   * the caches, and then caller needs to retry.
>   */
>   invalidate_local_connection (dbus_id, error);
>   return NULL;
> }

This connection comes from the GPrivate local connection cache:
connection = g_hash_table_lookup (local->connections, dbus_id);

GDaemonVfs::mount_cache is shared between all threads if I'm not mistaken.

This means if we do sync cdda IO from one thread which we keep alive, then eject the CD, and then try to do sync cdda IO from a different thread, GDaemonVfs::mount_cache will have the mount info from the first cdda access, but during the second access, local->connections will not contain the connection used for the first cdda access as it's a different thread, so we won't notice it went stale.

If GDBusCOnnection::closed can indeed come from a random thread, it may indeed be unsafe in some cases. 
_g_daemon_vfs_invalidate_dbus_id is thread-safe, so maybe I can use a signal handler only calling this, and pass a strdup'ed string as its user_data (and use g_signal_connect_data)

::: client/gvfsdaemondbus.c
@@ +498,3 @@
+
+  data = g_object_get_data (G_OBJECT (connection), "connection_data");
+  g_assert (data != NULL);

Sure, I tried to mirror what is done for async connections, but I initially tried doing what you suggest, I can go back to it.

Created attachment 267520 [details] [review]
Reworked version of one of the patches

Review of attachment 267520 [details] [review]:

Does this fix the issue, because it looks very good to me.

Review of attachment 267520 [details] [review]:

This fixes the issue sound-juicer is hitting (sync cdda IO from a thread, thread dies, new CD, sync cdda IO from a new thread).

This does not fix the secondary issue described in https://bugzilla.gnome.org/show_bug.cgi?id=602855#c10 which does not impact sound-juicer but could happen to others (sync cdda IO from a thread, thread stays alive, new CD, sync cdda IO from a new thread).

Ok, I see, but i'm not sure the other patch is right either.

So, you have this situation: 
Thread A does a sync call to the mount, this caches the connection locally in thread A and the mount info globally. Then thread A does something else, say loops calculating things.

During that loop the mount goes away. The gdbus thread detects this and sends an idle to the main context of the thread-default main context at the time of the construction of the connection. If the app didn't do anything this will be the main thread, if the app set a specific context it may be another thread. But in general there no guarantee that there is some mainloop looping on this context, so the close might not be emitted (ever, or in a while).

Now thread B comes along and does a sync call to the same mount. It has no local connection, but there *is* a stale global mount info cache. It tries to do a call on a GFile, which ends up in e.g. 
create_proxy_for_file2() which calls _g_dbus_connection_get_sync() with the stale mount_info->dbus_id.

This call returns the error:

Error: Cannot access CD: Error while getting peer-to-peer dbus connection: The
name :1.1447 was not provided by any .service files

Honestly, even if the app *did* service the mainloop this could be reached in race conditional states.

I think the correct way would be for the "dbus id is stale" state to be detected from _g_dbus_connection_get_sync(), and all callers of it has to be aware of this possibility, and then they have to re-get the mount infos. Thats the only race free solutions.

Review of attachment 267520 [details] [review]:

Actually, on second thought, this doesn't seem right.
Any time a thread exits it would invalidate the mount info cache for all other threads.
It should only invalidate if the connection has been closed.

*** Bug 606523 has been marked as a duplicate of this bug. ***

Comment on attachment 267520 [details] [review]
Reworked version of one of the patches

Setting patch status as per comment 15.

-- 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/gvfs/issues/127.