After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 708744 - GDaemonFileEnumerator registers itself on *all* dbus connections
GDaemonFileEnumerator registers itself on *all* dbus connections
Status: RESOLVED FIXED
Product: gvfs
Classification: Core
Component: client module
unspecified
Other Linux
: Normal normal
: ---
Assigned To: gvfs-maint
gvfs-maint
: 708721 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2013-09-25 12:53 UTC by Alexander Larsson
Modified: 2013-10-03 11:41 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
GDaemonFileEnumerator: Only listen to messages one connection (7.59 KB, patch)
2013-09-26 10:38 UTC, Alexander Larsson
committed Details | Review
GDaemonFileMonitor: Only recieve events on one dbus connection (4.29 KB, patch)
2013-09-26 10:38 UTC, Alexander Larsson
committed Details | Review
client: Remove unused code for dbus vfs filters (6.95 KB, patch)
2013-09-26 10:38 UTC, Alexander Larsson
committed Details | Review
client: Don't user g_source_remove() on non-default main context (3.18 KB, patch)
2013-10-03 11:40 UTC, Alexander Larsson
committed Details | Review

Description Alexander Larsson 2013-09-25 12:53:01 UTC
g_daemon_file_enumerator_new() calls:
  _g_dbus_register_vfs_filter (path,
                               register_vfs_filter_cb,
                               G_OBJECT (daemon));

and later (e.g in create_proxy_for_file2) we call _g_dbus_connect_vfs_filters() for *all* dbus connections used by any dbus thread (e.g. for each per-thread-and-per-mountpoint peer-to-peer connection).

This means register_vfs_filter_cb will be called in random thread using gvfs for each dbus connection ever used, where it will create a new skeleton.

I don't think this makes any sense. The callbacks from a single enumerate_children() will all be on a particular connection (i.e. the one we sent the request to), there is no need for all the others.
Comment 1 Alexander Larsson 2013-09-26 10:36:44 UTC
*** Bug 708721 has been marked as a duplicate of this bug. ***
Comment 2 Alexander Larsson 2013-09-26 10:37:16 UTC
Turns out that this is not only wasteful, but can lead to threadsafety issues as per bug 708721.
Comment 3 Alexander Larsson 2013-09-26 10:38:31 UTC
Created attachment 255809 [details] [review]
GDaemonFileEnumerator: Only listen to messages one connection

There is no need to listen to messages on other connections than
the one we sent the request on. In fact, doing so is bad as it can cause
locking issues as per bug 708721.
Comment 4 Alexander Larsson 2013-09-26 10:38:34 UTC
Created attachment 255810 [details] [review]
GDaemonFileMonitor: Only recieve events on one dbus connection

There is no need to register for monitor evens on all dbus connections,
just the one we send the request on.
Comment 5 Alexander Larsson 2013-09-26 10:38:38 UTC
Created attachment 255811 [details] [review]
client: Remove unused code for dbus vfs filters

This code is not used anymore
Comment 6 Alexander Larsson 2013-09-26 11:00:20 UTC
Attachment 255809 [details] pushed as 65b8b93 - GDaemonFileEnumerator: Only listen to messages one connection
Attachment 255810 [details] pushed as a302823 - GDaemonFileMonitor: Only recieve events on one dbus connection
Attachment 255811 [details] pushed as 8d08c55 - client: Remove unused code for dbus vfs filters
Comment 7 Sven Neumann 2013-09-26 13:22:26 UTC
I haven't done much testing yet, but this looks very good. Thank you!

You probably also want to remove the global obj_path_map in gvfsdaemonbus.c as it is not any longer used after your changes.
Comment 8 Ondrej Holy 2013-09-26 13:25:48 UTC
Removed by the Bug 591228.
Comment 9 Thaddäus Tintenfisch 2013-10-01 10:48:58 UTC
This patch appears to cause some trouble when using Thunar or PCManFM in the current Ubuntu development release (gvfs 1.18.1).

https://bugs.launchpad.net/ubuntu/+source/gvfs/+bug/1231978
Comment 10 Alexander Larsson 2013-10-03 06:56:10 UTC
Ok, repopening then. Do we have a backtrace of the hangs?
Comment 11 Alexander Larsson 2013-10-03 11:40:47 UTC
Created attachment 256356 [details] [review]
client: Don't user g_source_remove() on non-default main context

We created a GSource, attached it to a non-default main context and
then removed it via g_source_remove, which removed the source with the
same id on the main context, which typically was the X fd, causing
everything to stop working!

Non-default main contexts must use g_source_destroy()
Comment 12 Alexander Larsson 2013-10-03 11:41:22 UTC
Attachment 256356 [details] pushed as 33df421 - client: Don't user g_source_remove() on non-default main context