5) In g_test_dbus_dispose(), we do:

   if (self->priv->up)
    g_test_dbus_down (self);

So while we're in the middle of a GObject dispose hander, we're:

 * Acquiring a new reference to the bus GObject (assuming it exists, and let's for this discussion assume it does)
 * Sending SIGKILL to the bus Unix process
 * Synchronously calling write on a pipe to a separate helper process
 * Adding a weak reference on the bus GObject (note g_object_weak_ref() is not thread safe btw)
 * Dropping the reference obtained at the first step in an idle
 * Synchronously waiting until the bus GObject is destroyed

Now...really, we shouldn't be blocking in dispose at all, much less blocking on GObject refcounts.

Ok, so the actions test is failing for me because GTestDBus uses GFile, which of course calls into GVfs, which initializes a session bus behind its back:

(gdb) bt

+ Trace 230429

The thing I don't understand is why this test is just failing for me.  Do other people somehow have GIO_USE_VFS=local in their environment?

Created attachment 217255 [details] [review]
GTestDBus: Don't call into gvfs

Fundamentally, GTestDBus clashes badly with the GTest philosophy of having many unit tests in the same process.  Even if we take the above patch, it will still break horribly if you have:

[tests that use session bus, if indirectly]
[tests that use GTestDBus]
[tests that use session bus, if indirectly]

etc.  And due to GVfs, the "indirectly" can be very indirect.

Personally, my suggestion is to deprecate it, with no replacement inside GLib.  We can ship a cleaned up version of run-with-session-bus.sh in dbus itself, or fix up dbus-launch.

I don't think run-with-session-bus.sh or dbus-launch are going to cut it when the goal is to test scenarios where the bus is appearing or going away during the test.

(In reply to comment #0)
> I'm trying to debug why gio/tests/actions is failing with this symptom:
> 
> (/src/glib/gio/tests/.libs/lt-actions:18165): GLib-GIO-WARNING **: Weak notify
> timeout, object ref_count=1
> 
> 
> This seems to be a refcounting issue on the bus.  While debugging this, I've
> had to try to understand what is going on in GTestDBus.  This bug report will
> list the issues as I find them.
> 
> 1) Calling g_setenv/g_unsetenv() in a multi-threaded process is broken.  See
> https://bugzilla.gnome.org/show_bug.cgi?id=659326

I would say you are supposed to use g_test_dbus_up/down() from the main thread. The typical usage is puting them around g_test_run() in main(). So no dbus/thread code before/after.

I guess we should either document it that way, or make it threadsafe somehow.

> 2) #ifdef G_OS_WIN32
>                             /* We Need this to get the pid returned on win32 */
>                             G_SPAWN_DO_NOT_REAP_CHILD |
> #endif
> 
> Is broken.  We don't know the lifetime of the pid unless we have a watch. 
> Really gspawn.c should have a g_return_if_fail on this.

I have 0 knowledge of win32. I don't know what's the problem here :)

> 3) There's Windows-specific code to attempt to kill the bus if the test
> crashes, but this code is missing the Linux equivalent -
> prctl(PR_SET_PDEATHSIG).

The linux code has a child watcher process that will kill the bus process when parent dies. I don't know if there is a cleaner way of doing this, but that's how GDBus unit tests always did.

> 4) Code assumes bus addresses don't have spaces/shell metacharacters:
> "dbus-monitor --address %s"

Right, that's how GDBus unit tests always did. Fix welcome :)

(In reply to comment #1)
> 5) In g_test_dbus_dispose(), we do:
> 
>    if (self->priv->up)
>     g_test_dbus_down (self);
> 
> So while we're in the middle of a GObject dispose hander, we're:
> 
>  * Acquiring a new reference to the bus GObject (assuming it exists, and let's
> for this discussion assume it does)
>  * Sending SIGKILL to the bus Unix process
>  * Synchronously calling write on a pipe to a separate helper process
>  * Adding a weak reference on the bus GObject (note g_object_weak_ref() is not
> thread safe btw)
>  * Dropping the reference obtained at the first step in an idle
>  * Synchronously waiting until the bus GObject is destroyed
> 
> Now...really, we shouldn't be blocking in dispose at all, much less blocking on
> GObject refcounts.

True... I would suggest to change that to a g_assert (!self->priv->up); so tests will always have to explicitly call g_test_dbus_down() before unreffing. Which is saner IMO, and IIRC it's already documented that they SHOULD do that.

(In reply to comment #5)
> Fundamentally, GTestDBus clashes badly with the GTest philosophy of having many
> unit tests in the same process.  Even if we take the above patch, it will still
> break horribly if you have:
> 
> [tests that use session bus, if indirectly]
> [tests that use GTestDBus]
> [tests that use session bus, if indirectly]
> 
> etc.  And due to GVfs, the "indirectly" can be very indirect.

Why would it clash? Any proper unit tests using dbus should ensure that when it leaves no singleton are still alive and reused in the next test. So even if GDBus is used indirectly it must ends with the GDBusConnection singleton set to NULL.

The only difference now is that sane behaviour is somewhat made mandatory.

> Personally, my suggestion is to deprecate it, with no replacement inside GLib. 
> We can ship a cleaned up version of run-with-session-bus.sh in dbus itself, or
> fix up dbus-launch.

.sh wrapper are really evil. It means that when you start your unit test with ./tests/foo, without passing by an obscrure "make check" incantation, it will start using your session bus and mess up things. And there is nothing a bash wrapper can do that we don't do with C API... Again GTestDBus API is really meant to be used like:

main()
{
  GTestDBus *dbus;

  dbus = g_test_dbus_new ();
  g_test_dbus_up (dbus);

  ...

  ret = g_test_dbus_run ();

  g_test_dbus_down (dbus);
  g_object_unref (dbus);

  return ret;
}

Oh, btw if we really want to remove GTestDBus, I don't think it has to deprecated, GIO does not guarantee API/ABI stability in dev branch, right?

Review of attachment 217255 [details] [review]:

I'm 100% sure it was working before, and I doubt my GVFS is local only.

I'm wondering if your problem is actually a leak in gvfs itself! When the g_file_delete() operation finishes, the GDBusConnection singleton should get reset to NULL, no? So there is no reason why it would influence the unit test afterward.

Comment on attachment 217255 [details] [review]
GTestDBus: Don't call into gvfs

Attachment 217255 [details] pushed as d6aa3b3 - GTestDBus: Don't call into gvfs

(In reply to comment #5)
> [tests that use session bus, if indirectly]
> [tests that use GTestDBus]
> [tests that use session bus, if indirectly]
> 
> etc.  And due to GVfs, the "indirectly" can be very indirect.

Obviously the answer is that we don't want any "tests that use session bus" (even if indirectly). Otherwise we can't run them from a buildbot or git commit hook etc etc.

There are various ways to enforce this (clearing DBUS_SESSION_BUS_ADDRESS before running test code is one of them).

> Personally, my suggestion is to deprecate it, with no replacement inside GLib. 
> We can ship a cleaned up version of run-with-session-bus.sh in dbus itself, or
> fix up dbus-launch.

I don't think there's any need for such drama - GTestDBus is definitely very useful outside GLib. It could it needs some cleanup here or there and your work on this is appreciated. Thanks.

(In reply to comment #10)
> Review of attachment 217255 [details] [review]:
> 
> I'm 100% sure it was working before, and I doubt my GVFS is local only.
> 
> I'm wondering if your problem is actually a leak in gvfs itself! When the
> g_file_delete() operation finishes, the GDBusConnection singleton should get
> reset to NULL, no? So there is no reason why it would influence the unit test
> afterward.

My money is on Colin using the new GVfs which has Tomas' GDBus port... Colin: can you confirm this?

I think we should disable GIO module loading for all tests (except for those testing the module loading facility, of course)....

(In reply to comment #12)

> Obviously the answer is that we don't want any "tests that use session bus"
> (even if indirectly). Otherwise we can't run them from a buildbot or git commit
> hook etc etc.
> 
> There are various ways to enforce this (clearing DBUS_SESSION_BUS_ADDRESS
> before running test code is one of them).

I agree. It provides a more controlled test environment, as well as being more robust about accidentally changing/destroying the user session, settings, etc.

> > Personally, my suggestion is to deprecate it, with no replacement inside GLib. 
> > We can ship a cleaned up version of run-with-session-bus.sh in dbus itself, or
> > fix up dbus-launch.
> 
> I don't think there's any need for such drama - GTestDBus is definitely very
> useful outside GLib. It could it needs some cleanup here or there and your work
> on this is appreciated. Thanks.

Full ack. I love having it, it makes writing tests quite a bit easier than having to integrate dbus-launch into Makefiles, and remembering to run it in other circumstances. I'd rather make it easier/more robust in the sense of setting $DBUS_SESSION_BUS_ADDRESS in the environment (and perhaps remembering the original value), and adding simple flag to also export it to $DBUS_SYSTEM_BUS_ADDRESS; the latter is already easy to do, but one needs to remember the variable name and doing it in the first place if you want to test system services on a mock session bus in tests.

(In reply to comment #13)
> I think we should disable GIO module loading for all tests (except for those
> testing the module loading facility, of course)....

For glib's "make check" tests themselves I think that's a good idea indeed. We do want the modules for system/integration tests and for e. g. writing gvfs tests, but not for Gio itself.

Thanks!

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

Created attachment 258451 [details] [review]
GTestDBus: document that g_test_dbus_down() might hang because of modules

Review of attachment 258451 [details] [review]:

::: gio/gtestdbus.c
@@ +798,3 @@
+ * that problem it is recommended to use the "memory" backend of GSettings and
+ * the "local" backend of GVFS. A system could be added in the future to prevent
+ * module loading in test environment, but in the meantime it can be done by

"in test environment" => "in a test environment"

@@ +799,3 @@
+ * the "local" backend of GVFS. A system could be added in the future to prevent
+ * module loading in test environment, but in the meantime it can be done by
+ * adding this in your makefile: <informalexample><programlisting>

"makefile" => "Makefile.am" or similar

this should probably point out that this is for the automake test framework, and won't do magically do anything on its own:

    ... but in the meantime, make sure that you run your tests
    with the envvars GIO_USE_VFS=local and GSETTINGS_BACKEND=memory.
    In the automake test framework, this can be done by setting
    TESTS_ENVIRONMENT in your Makefile.am:

    ...

-- 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/563.