Re: gdbus branch review

[Alexander Larsson <alexl redhat com>]

On Mon, 2012-05-21 at 15:45 +0200, Tomas Bzatek wrote:
> On Mon, 2012-04-23 at 15:39 +0200, Alexander Larsson wrote:
> > * metadata (client/gdaemonvfs.c client/gdaemonfile.c):
> > 
> > The proxy for the metadata daemon is recreated a lot, it should be
> > constructed once then then kept around in the GVfs for future use. This
> > is especially problematic since constructing the proxy does sync i/o,
> > and this is done in g_daemon_vfs_local_file_moved/removed which is
> > called for every local file remove, making them sync even if they are
> > async.
> 
> Oh, I think I've been facing the excessive proxy creation before, on
> Midori offline cache cleaning. Anyway, I've pushed the metadata proxy in
> a GPrivate instance, so we have separate proxies for every thread. While
> GDBusProxy should be thread-safe, its creation and check for NULL would
> have to be surrounded by locks.

Really? Why not create it in the GVfs initializer? That should be
protected by a lock already. Or if you want to do it lazily, use a
g_once_init_* thing?

I worry a bit about the GPrivate use with
g_bus_watch_name_on_connection(). On which thread is the
metadata_daemon_vanished callback called? Are we guaranteed that it'll
be on the thread that had the GPrivate? That seems unlikely as it might
not even run a mainloop...

> For the sync call in async ops thing, I don't see any use of the
> affected methods in async calls, all seem to be called from GIO sync
> ones. Since they're having "local_file_" prefix I don't expect them to
> be called from somewhere else.
> 
> Async variants of the calls use g_simple_async_result_run_in_thread() to
> make a sync operation async. So I think we're on safe side here.

True.

> > Furthermore, for the g_daemon_vfs_local_file_moved/removed case the
> > actual method call is sync too. This further adds sync i/o on a possibly
> > async codepath. This used to be a send + flush + ignore result
> > (send_message_oneway), and need to still be.
> 
> Got your point, waiting for answer is not necessary, we're not expecting
> any value back. Changed to async calls, the only risk here is a missing
> flush call. If we use g_dbus_connection_flush_sync() to ensure the
> message got actually out, we might face accidental flush of tons of
> messages from other places and threads as long as session bus connection
> is shared in this case. Not sure which way is better.

If you don't flush, then a commandline invocation like gvfs-copy will
exit with the dbus outgoing message queue non-empty, and we will thus
lose the metadata change. I agree that a flush might be unnecessarily
costly though. Not sure what the best approach is here. Maybe GDBus
could do a flush in an atexit handler? Or maybe the gvfs commandline
utils should ensure flushing on exit?