Created attachment 259654 [details] [review]
gtestdbus: Fix variable shadowing

Shut up, GCC.

Created attachment 259655 [details] [review]
gtestdbus: Fix non-const use of const variables

The argv array should be declared as const.

Created attachment 259656 [details] [review]
gtestdbus: Minor documentation fixes

Created attachment 259657 [details] [review]
gtestdbus: Add support for starting isolated system buses

Add a new G_TEST_DBUS_SYSTEM_BUS flag which can be passed to
g_test_dbus_new() to create a system bus instead of a session bus. This
handles setting the correct environment variables, and ensures that the
bus’ security policy is completely permissive.

This has the problem that g_test_dbus_unset() is called by g_test_dbus_up(), and it unsets all previous system and session bus addresses, so code which creates two GTestDBus instances (session and system) will end up having the environment variables for one unset when the second is created.

The only solution I can see to this is to remove the g_test_dbus_unset() call from g_test_dbus_up() which is a behaviour change, but not one which violates the documentation for g_test_dbus_up().

Created attachment 259660 [details] [review]
gtestdbus: Selectively unset D-Bus environment variables

Don’t always unset all D-Bus environment variables when starting up or
shutting down an isolated bus, as to do so might trample on other
GTestDBus instances for other kinds of bus (e.g. session vs. system).

Instead, always unset a few common environment variables (like DISPLAY),
but only unset the DBUS_*_BUS_ADDRESS variable corresponding to the type
of bus being start up or shut down.

Created attachment 259744 [details] [review]
gtestdbus: Fix warning about removing a non-existent source

If the timeout expires, the code still attempts to remove the source
from the main loop, even though this has already been done automatically
upon expiration of the timeout.

Clear the source ID when the timeout is fired to prevent this.

Created attachment 259786 [details] [review]
gtestdbus: Add support for starting isolated system buses

Add a new G_TEST_DBUS_SYSTEM_BUS flag which can be passed to
g_test_dbus_new() to create a system bus instead of a session bus. This
handles setting the correct environment variables, and ensures that the
bus’ security policy is completely permissive.

Created attachment 259787 [details] [review]
gtestdbus: Selectively unset D-Bus environment variables

Don’t always unset all D-Bus environment variables when starting up or
shutting down an isolated bus, as to do so might trample on other
GTestDBus instances for other kinds of bus (e.g. session vs. system).

Instead, always unset a few common environment variables (like DISPLAY),
but only unset the DBUS_*_BUS_ADDRESS variable corresponding to the type
of bus being start up or shut down.

Created attachment 259788 [details] [review]
gtestdbus: Fix warning about removing a non-existent source

If the timeout expires, the code still attempts to remove the source
from the main loop, even though this has already been done automatically
upon expiration of the timeout.

Clear the source ID when the timeout is fired to prevent this.

Created attachment 259790 [details] [review]
gtestdbus: Unset the DBUS_SESSION_BUS_[PID|WINDOWID] variables

Might as well do a complete job when unsetting environment variables
which are associated with D-Bus.

Created attachment 262920 [details] [review]
gtestdbus: Fix variable shadowing

Shut up, GCC.

Created attachment 262921 [details] [review]
gtestdbus: Fix non-const use of const variables

The argv array should be declared as const.

Created attachment 262922 [details] [review]
gtestdbus: Minor documentation fixes

Created attachment 262923 [details] [review]
gtestdbus: Add support for starting isolated system buses

Add a new G_TEST_DBUS_SYSTEM_BUS flag which can be passed to
g_test_dbus_new() to create a system bus instead of a session bus. This
handles setting the correct environment variables, and ensures that the
bus’ security policy is completely permissive.

Created attachment 262924 [details] [review]
gtestdbus: Selectively unset D-Bus environment variables

Don’t always unset all D-Bus environment variables when starting up or
shutting down an isolated bus, as to do so might trample on other
GTestDBus instances for other kinds of bus (e.g. session vs. system).

Instead, always unset a few common environment variables (like DISPLAY),
but only unset the DBUS_*_BUS_ADDRESS variable corresponding to the type
of bus being start up or shut down.

Created attachment 262925 [details] [review]
gtestdbus: Unset the DBUS_SESSION_BUS_[PID|WINDOWID] variables

Might as well do a complete job when unsetting environment variables
which are associated with D-Bus.

Created attachment 262926 [details] [review]
gtestdbus: Add a G_TEST_DBUS_SESSION_BUS flag

This mirrors the G_TEST_DBUS_SYSTEM_BUS flag, and can be used to clarify
which bus is expected to be created. The default behaviour (if
G_TEST_DBUS_SYSTEM_BUS isn’t specified) is to create a session bus,
however, so setting this flag isn’t at all required.

Review of attachment 262920 [details] [review]:

ok

Review of attachment 262920 [details] [review]:

ok

Review of attachment 262921 [details] [review]:

ok

Review of attachment 262922 [details] [review]:

sure

Comment on attachment 259788 [details] [review]
gtestdbus: Fix warning about removing a non-existent source

Stef already fixed this in master.

Attachment 262920 [details] pushed as 598a9c5 - gtestdbus: Fix variable shadowing
Attachment 262921 [details] pushed as 46c1aea - gtestdbus: Fix non-const use of const variables
Attachment 262922 [details] pushed as 9d4cd9c - gtestdbus: Minor documentation fixes

Review of attachment 262924 [details] [review]:

::: gio/gtestdbus.c
@@ +700,3 @@
+  g_unsetenv ("DISPLAY");
+  g_unsetenv ("DBUS_STARTER_ADDRESS");
+  g_unsetenv ("DBUS_STARTER_BUS_TYPE");

I would prefer having a common_envar_unset() for those, and call it from g_test_dbus_unset() as well. This would avoid duplicate places where to add future envars, like you do in the following patch.

@@ +712,3 @@
+      case G_BUS_TYPE_STARTER:
+        /* Always want to unset the starter address since we don't support
+         * simulating auto-launched buses */

Not sure why this comment is here, it's not here that it unset STARTER. I guess it should go to the common_envar_unset() I suggested.

Also, I'm not sure what's the glib policy here, but if you have a default in a switch, there is no need to have cases that fallback to it.

Review of attachment 262926 [details] [review]:

::: gio/gioenums.h
@@ +1714,1 @@
   G_TEST_DBUS_SYSTEM_BUS,

It is not obvious that those are flag values, I would add "= 1 << 0" and " = 1 << 1"

What about:

typedef enum /*< flags >*/ {
  G_TEST_DBUS_SESSION_BUS = 0,
  G_TEST_DBUS_SYSTEM_BUS = 1 << 0,
}

/* Old flag we have to keep for API compatibility */
#define G_TEST_DBUS_NONE G_TEST_DBUS_SESSION

::: gio/gtestdbus.c
@@ +565,3 @@
           "  <type>system</type>\n");
     }
+  else /* if (self->priv->flags & G_TEST_DBUS_SESSION_BUS) */

I think you should uncomment this, and add something like: else {g_return_if_reached();}

The rest looks good to me. Thanks.

Note that all use of setenv() or unsetenv() is totally unsafe in the presence of threads (for example, the GDBus thread).

See https://bugzilla.gnome.org/show_bug.cgi?id=659326

(Personally I think GTestDBus is well-intentioned crack, of which this is just one problem)

There is no reason to use GTestDBus in threads. You create it before launching threads.

Created attachment 263376 [details] [review]
gtestdbus: Add support for starting isolated system buses

Add a new G_TEST_DBUS_SYSTEM_BUS flag which can be passed to
g_test_dbus_new() to create a system bus instead of a session bus. This
handles setting the correct environment variables, and ensures that the
bus’ security policy is completely permissive.

Created attachment 263377 [details] [review]
gtestdbus: Selectively unset D-Bus environment variables

Don’t always unset all D-Bus environment variables when starting up or
shutting down an isolated bus, as to do so might trample on other
GTestDBus instances for other kinds of bus (e.g. session vs. system).

Instead, always unset a few common environment variables (like DISPLAY),
but only unset the DBUS_*_BUS_ADDRESS variable corresponding to the type
of bus being start up or shut down.

Created attachment 263378 [details] [review]
gtestdbus: Unset the DBUS_SESSION_BUS_[PID|WINDOWID] variables

Might as well do a complete job when unsetting environment variables
which are associated with D-Bus.

Created attachment 263379 [details] [review]
gtestdbus: Add a G_TEST_DBUS_SESSION_BUS flag

This mirrors the G_TEST_DBUS_SYSTEM_BUS flag, and can be used to clarify
which bus is expected to be created. The default behaviour (if
G_TEST_DBUS_SYSTEM_BUS isn’t specified) is to create a session bus,
however, so setting this flag isn’t at all required.

Created attachment 263380 [details] [review]
gtestdbus: Add a note about thread safety to the documentation

(In reply to comment #25)
> Review of attachment 262924 [details] [review]:
> I would prefer having a common_envar_unset() for those, and call it from
> g_test_dbus_unset() as well. This would avoid duplicate places where to add
> future envars, like you do in the following patch.

Done.

> Also, I'm not sure what's the glib policy here, but if you have a default in a
> switch, there is no need to have cases that fallback to it.

I find it documents the fact that the programmer has actually considered those cases, and not just forgotten about them. It’s the -Wswitch-enum warning from GCC. I don’t think GLib has a policy on it.

(In reply to comment #26)
> Review of attachment 262926 [details] [review]:
> It is not obvious that those are flag values, I would add "= 1 << 0" and " = 1
> << 1"

Tidied up.

> ::: gio/gtestdbus.c
> @@ +565,3 @@
>            "  <type>system</type>\n");
>      }
> +  else /* if (self->priv->flags & G_TEST_DBUS_SESSION_BUS) */
> 
> I think you should uncomment this, and add something like: else
> {g_return_if_reached();}

If (G_TEST_DBUS_SESSION_BUS == 0) that won’t work at all. I’ve removed the comment completely, since it’s fairly obvious what’s going on.

(In reply to comment #29)
> There is no reason to use GTestDBus in threads. You create it before launching
> threads.

It’s quite possible you’d want to put the bus up and down between (e.g.) unit tests inside a test binary, but you’re in for a world of pain if you’ve still got computation going on when you do that anyway, so GTestDBus isn’t making things worse.

Those patches looks good to me, but I'm not a glib maintainer.

Review of attachment 263380 [details] [review]:

Okay...but really most GLib users are going to at least have the GLib worker thread and the GDBus thread, many will have the GSettings thread...

(In reply to comment #37)
> Review of attachment 263380 [details] [review]:
> 
> Okay...but really most GLib users are going to at least have the GLib worker
> thread and the GDBus thread, many will have the GSettings thread...

Dunno what the "glib worker" thread is, when is it started/stopped?

GDBus thread should obviously not be started before GTestDBus.

Tests should use the "memory" backend of GSettings and "local" backend of GIO, are they using threads? I think modules should be generally disabled in test env, I think glib should provide tools for that. See https://bugzilla.gnome.org/show_bug.cgi?id=678808#c16.

Comment on attachment 263380 [details] [review]
gtestdbus: Add a note about thread safety to the documentation

Colin, what are your thoughts on the rest of the patches? This is needed for some unit testing in libfolks, and I’m sure it’ll be useful anywhere else some unit tests need to mock a system bus object.

Attachment 263380 [details] pushed as c07eccd - gtestdbus: Add a note about thread safety to the documentation

<Services> Bug 712148: enhancement, Normal, ---, zeuthen, UNCONFIRMED, Add system bus support to GTestDBus
<walters> pwithnall: i'd prefer to pretend GTestDBus doesn't exist, can you toss the patches to say pitti or desrt?
<pwithnall> pitti, desrt: ^
<walters> (i think using dbus-launch externally to one's test program is better)
<desrt> pwithnall: uh... i'd also prefer ... :)
<larsu> walters: interesting. Any specific reason for that or do you just think it's cleaner?
<walters> larsu: https://bugzilla.gnome.org/show_bug.cgi?id=712148#c28 
<Services> Bug 712148: enhancement, Normal, ---, zeuthen, UNCONFIRMED, Add system bus support to GTestDBus
<larsu> walters: ah good point, thanks
<pwithnall> walters: dbus-launch doesn’t do system busses, does it?
<walters> pwithnall: no...but you could do env MYPROJECT_USE_SESSION_BUS_AS_SYSTEM=1 ./mytestcode and inside your code do g_bus_get (g_getenv ("...AS_SYSTEM") ? G_BUS_SESSION : G_BUS_SYSTEM);
<walters> or just set DBUS_SYSTEM_BUS_ADDRESS to your custom bus address
<pwithnall> ewww
<pwithnall> imo, that’s as icky as GTestDBus
<pwithnall> At least with GTestDBus all the ickiness is in one place, rather than duplicated across every project
<walters> things not being in glib doesn't imply they need to be copy and paste or rewritten each time
<walters> it turns out glib is not the only shared or sharable library...
<pwithnall> This is true
<pwithnall> But having GTestDBus in GLib somewhat suggests people should standardise on it
<pwithnall> I think you should either review/accept the patch and keep GTestDBus, or deprecate GTestDBus entirely if you don’t want people to use it
<pwithnall> As it stands, folks has had a huge patchset blocked for a month because nobody’s replied on that bug saying the patch isn’t wanted :(
<walters> let's use the term "we" here rather than "you"...it's a collaborative project =)
<pwithnall> I’m not a GLib maintainer, and I can’t review my own patches even if I were :)
<walters> i'm certainly not personally blocking the patches, if someone else +1s them I would not complain if they went in
<walters> i'd just prefer not to lend tacit endorsement of GTestDBus by reviewing them myself
<pwithnall> I understand
<pwithnall> from the original bug (https://bugzilla.gnome.org/show_bug.cgi?id=672985), looks like mclasen and davidz originally acked GTestDBus
<Services> Bug 672985: normal, Normal, ---, zeuthen, RESOLVED FIXED, Move gdbus-sessionbus to GTest
<mclasen> in retrospect, having it in a separate binary would be nicer
* chpe is now known as chpe[afk]
<pwithnall> mclasen: I suggest it would be easiest to commit the system bus stuff to GTestDBus first, then split it out into a separate binary
<pwithnall> to avoid the patches bitrotting
<desrt> pwithnall: i'm not really sure how it would be possible to do that...
<pwithnall> Do what?
<desrt> it's API... you can't just move that to a separate process
* pwithnall was assuming mclasen meant a separate library
<desrt> and it modifies the existing process in-place
<desrt> pwithnall: i don't think that this would really solve the problems at all
<pwithnall> desrt: What would you suggest then?
<desrt> honestly, i wouldn't mind deprecating the entire thing as being fundamentally broken
<desrt> and seeing if we can find a way to reduce the size of it as much as possible -- it was a mistake
* desrt never liked it either
<desrt> i certainly don't think we should be adding features to it
<pwithnall> Just because of the envvar races?
<desrt> that's a pretty fundamental problem
<desrt> we have periodically failing test cases because of this... today
<pwithnall> hum
<desrt> the gdbus-name test will fail if you run it in a loop due to this insanity
<desrt> it also limits what you can do inside the library and the test program itself
<desrt> for example, if we wanted to move the sampling of the session bus address to a constructor to ensure we were running it from a no-threads environment, it would break GTestDBus
<pwithnall> What would your suggested replacement be then?
<desrt> it seems there are quite a few scripts hanging around that do this...
* hyperair (~hyperair@bb220-255-137-219.singnet.com.sg) has joined #gnome-hackers
<desrt> dbus-test-runner or something?
<pwithnall> Yes, we used them in libfolks and they were horrendous
<pwithnall> They made debugging and logging harder
<walters> dbus-run-session is in dbus upstream now btw
<walters> https://bugs.freedesktop.org/show_bug.cgi?id=39196
<Services> Bug 39196: normal, medium, ---, simon.mcvittie, RESOLVED FIXED, dbus-launch can't be told to exit with its child
<pwithnall> We were using a predecessor of dbus-run-session, nabbed from Telepathy
<pwithnall> Right. I guess I’m going to have to re-do this entire libfolks branch to not use GTestDBus; or to copy gtestdbus.c into the source tree locally, or something :(
<pwithnall> and I guess someone should update GTestDBus to mark it as deprecated, lest someone else fall into the same trap of using it
* pwithnall will try and find time to do that, but might not manage it for some days

I really don't understand what you guys have against GTestDBus. It has been immensely useful to me and others. I also don't understand what's the advantage of having yet-another-lib. We already have GTest in libglib.so...

I don't know what fails  with gdbus-names, but I don't think it is possible to implement those tests with external wrapper script. It tests what happens before the bus is started, and after it's stopped...

Restoring the bug title. If you have something against GTestDBus please open your own bug instead of stealing someone else's valid bug. Also please give detailed arguments instead of pasting some random long IRC discussion that does not explain anything.

(In reply to Xavier Claessens from comment #41)
> I really don't understand what you guys have against GTestDBus.

I do, let me try to explain...

GTestDBus manipulates global state (the environment) in a way that is not thread-safe: POSIX says setenv() isn't thread-safe, and if you think about how it would have to be implemented, bearing in mind that iteration over "extern char **environ" without holding a lock is part of the POSIX environment API, you'll probably conclude that there would be no way to make it thread-safe.

GTestDBus also affects global state (GDBus' global singleton session bus connection) in a way that is hard to reset: once you have connected to the session bus, other global singletons can be holding refs to it, preventing it from being released without teaching everything to cope with it being disposed, which is something that never happens "in real life".

That's fine up to a point, but GTest runs multiple test-cases in one process invocation: setup, test, teardown, setup, test, teardown.

You can do g_test_dbus_up() in setup(), fine; but when you try to do g_test_dbus_down() in teardown(), you can't guarantee that there aren't objects (perhaps singletons, perhaps in other threads) still using that bus, and you can almost guarantee that there *are* other threads (notably the GDBus worker thread), which means that the setenv() that g_test_dbus_down() needs to do is undefined behaviour.

Other implementations:

IIRC, Folks has historically dealt with this by doing g_test_up() in main() instead of per-test, and just trying really hard to clean up all other singletons in teardown() so they don't leak into subsequent tests (which has not always been successful).

dbus regression tests that exercise the dbus-daemon[1] have a "dbus-daemon on a stick" as a subprocess, similar to g_test_dbus_up(), but generally try to avoid manipulating DBUS_SESSION_BUS_ADDRESS or using singletons: instead, they pass that dbus-daemon's address around as a string, and connect to that address explicitly. This avoids the "global state" problem, but means you need to avoid singleton-based APIs - e.g. you have to use tp_account_manager_new() instead of tp_account_manager_dup(), even though the latter is the one that's encouraged in "real-life" code.

[1] by which I mean the GTest-based ones that I wrote, like test/dbus-daemon.c

Various projects wrap their regression tests in dbus-launch (discouraged, deprecated, don't do it) or dbus-run-session ("dbus-launch done right" for this particular purpose). This is a bit annoying when you want to use gdb or valgrind, but that can be worked around.

Putting each D-Bus test case in a g_test_trap_subprocess() subprocess, using g_test_dbus_up() or equivalent, and avoiding g_test_dbus_down() or equivalent is, again, annoying when you want to use gdb or valgrind, but possible.

(In reply to Xavier Claessens from comment #42)
> I don't know what fails  with gdbus-names, but I don't think it is possible
> to implement those tests with external wrapper script. It tests what happens
> before the bus is started, and after it's stopped...

This seems like a job for the dbus tests' approach: avoid singletons, avoid manipulating the environment, and take an "explicit is better than implicit" approach to the dbus-daemon.

Another possible approach to rescuing GTestDBus from undefined behaviour would be to have per-process initialization (global, in main() or the first setup()) to:

* create a secure temporary directory (to avoid DoS by other processes)
* compute a socket path within that directory (e.g. THATDIR/bus)
* compute a unix:path= address within that directory (with proper escaping)
* set that as DBUS_SESSION_BUS_ADDRESS
* start a helper subprocess to watch for the test's death, e.g.
  by passing the read end of a pipe to it

then in setup():

* delete a stale socket if necessary
* spawn a dbus-daemon hard-coded to listen at that precise address
* connect the singleton connection to it
* turn off exit-on-disconnect

and in teardown():

* kill the dbus-daemon
* wait for the singleton connection to be disconnected

and in the helper subprocess, when the main test process dies:

* kill the dbus-daemon if necessary
* wait for the dbus-daemon to exit (this would probably have to poll,
  which is sad)
* delete the socket if the dbus-daemon didn't do it already
* clean up the temporary directory

(In reply to Simon McVittie from comment #45)
> Another possible approach to rescuing GTestDBus from undefined behaviour
> would be to have per-process initialization (global, in main() or the first
> setup()) to:

I've thought before that it would be nice to be able to specify setup() and teardown() functions for a GTestSuite (ie, with the setup() func being run before the first test in the suite, and the teardown() after the last test in the suite). So then what you're suggesting would just be the setup/teardown functions for the "/" suite.

(Related: bug 722648 comment 6 and 7, which talks about trying to run all tests in subprocesses, which is incompatible with doing setup in main().)

-- 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/glib/issues/789.