GNOME Bugzilla – Bug 723506
fork/exec from non-main thread when autolaunching could be avoided when DISPLAY unset
Last modified: 2016-07-17 00:34:17 UTC
Created attachment 267904 [details] [review] Do not attempt to autolaunch a session dbus-daemon with no DISPLAY The two known use-cases for autolaunching are: * X-forwarding: "ssh -Y myhost myapp" resulting in a session bus on myhost but an X server on the original host * Legacy desktop environments on OSs without D-Bus integration: e.g. running a single GNOME or KDE app under fvwm or something, without a session dbus-daemon being started by either systemd, gnome-session, or OS integration scripts analogous to Debian's /etc/X11/Xsession.d/75dbus_dbus-launch In either case, an X11 DISPLAY is also needed. "dbus-launch --autolaunch" doesn't do anything useful when unable to connect to an X11 display; this has been the case since the feature was added in 2006, and is useful to avoid "split brain" situations in which two processes that ought to be part of the same session end up on separate session buses. Since dbus commit 407c111 in 2011, libdbus hasn't even attempted to run "dbus-launch --autolaunch" unless getenv("DISPLAY") returns non-null in the parent: this avoids doing a relatively complicated fork-and-exec that is clearly not going to lead to success. This commit gives GDBus the same policy. In Debian, I've encountered GLib test failures in gio/tests/gapplication.c which appear to be related to the fork-and-exec for dbus-launch (Debian bug #737380). Specifically, one test-case ends by calling g_test_dbus_unset(), and the next test-case tries to access the session bus with DBUS_SESSION_BUS_ADDRESS unset, resulting in an attempt to autolaunch. Checking for DISPLAY avoids this attempt to autolaunch, since g_test_dbus_unset() also unsets DISPLAY. It seems likely that there is some orthogonal bug here, perhaps involving g_spawn_sync() in the GDBus thread racing with g_spawn_async_with_pipes() in the main thread, but GApplication is probably not the ideal place to debug that. In my opinion as D-Bus maintainer, "dbus-launch --autolaunch" should be considered to be an X11 feature, and any future D-Bus enhancements (e.g. kdbus) or successors for X11 (e.g. Wayland, Mir) should obtain a session bus address by other means - either a session manager such as "systemd --user", gnome-session or Upstart, or a wrapper for the user session like dbus-run-session(1). Related to dbus bug <https://bugs.freedesktop.org/show_bug.cgi?id=19997>. Bug-Debian: http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=737380
(In reply to comment #0) > It seems likely that there is some orthogonal bug here, > perhaps involving g_spawn_sync() in the GDBus thread racing with > g_spawn_async_with_pipes() in the main thread, but GApplication is > probably not the ideal place to debug that. I might look into this when I'm more awake, but please don't count on it. In any case, simplifying what happens in the GDBus thread seems a good idea. Test environment: GLib 2.38.2 with Debian 2.38.2-2 patchset (none of which seems relevant to g_spawn_*), in an almost-minimal Debian unstable chroot on a mipsel porterbox.
(In reply to comment #0) > It seems likely that there is some orthogonal bug here, > perhaps involving g_spawn_sync() in the GDBus thread racing with > g_spawn_async_with_pipes() in the main thread See Bug #723743 for one thing I'm suspicious of.
(In reply to comment #0) > It seems likely that there is some orthogonal bug here, > perhaps involving g_spawn_sync() in the GDBus thread racing with > g_spawn_async_with_pipes() in the main thread It looks like that's Bug #711090.
Created attachment 302461 [details] [review] Do not attempt to autolaunch a session dbus-daemon with no DISPLAY The two known use-cases for autolaunching are: * X-forwarding: "ssh -Y myhost myapp" resulting in a session bus on myhost but an X server on the original host * Legacy desktop environments on OSs without D-Bus integration: e.g. running a single GNOME or KDE app under fvwm or something, without a session dbus-daemon being started by either systemd, gnome-session, or OS integration scripts analogous to Debian's /etc/X11/Xsession.d/75dbus_dbus-launch In either case, an X11 DISPLAY is also needed. "dbus-launch --autolaunch" doesn't do anything useful when unable to connect to an X11 display; this has been the case since the feature was added in 2006, and is useful to avoid "split brain" situations in which two processes that ought to be part of the same session end up on separate session buses. Since dbus commit 407c111 in 2011, libdbus hasn't even attempted to run "dbus-launch --autolaunch" unless getenv("DISPLAY") returns non-null in the parent: this avoids doing a relatively complicated fork-and-exec that is clearly not going to lead to success. This commit gives GDBus the same policy. This change was originally made to work around a race condition in subprocess spawning (Debian bug #737380, GNOME bug #711090) but it seems valid in its own right. In my opinion as D-Bus maintainer, "dbus-launch --autolaunch" should be considered to be an X11 feature, and any future D-Bus enhancements (e.g. kdbus) or successors for X11 (e.g. Wayland, Mir) should obtain a session bus address by other means - either a session manager such as "systemd --user", gnome-session or Upstart, or a wrapper for the user session like dbus-run-session(1). Related to dbus bug <https://bugs.freedesktop.org/show_bug.cgi?id=19997>. --- Refreshed the commit message with what I have now concluded about the original Debian bug. No functional changes. See also Bug #747941, which adds a more-preferred default (XDG_RUNTIME_DIR/bus) to be tried before `dbus-launch --autolaunch`.
(In reply to Simon McVittie from comment #4) > Do not attempt to autolaunch a session dbus-daemon with no DISPLAY We've been using this in Debian for about a year. May I apply it upstream? (desrt, perhaps you have thoughts on this?)
Review of attachment 302461 [details] [review]: Sorry no one reviewed this earlier. I think GDBus should conceptually follow what libdbus does, so since libdbus does it now (and we agreed there), it should follow that GDBus changes. LGTM.
The following fixes have been pushed: