| Summary: | add regression test for using a non-default mainloop context | ||
|---|---|---|---|
| Product: | dbus | Reporter: | DavidT <david.tucker> |
| Component: | GLib | Assignee: | Simon McVittie <smcv> |
| Status: | RESOLVED FIXED | QA Contact: | John (J5) Palmieri <johnp> |
| Severity: | normal | ||
| Priority: | medium | CC: | rob.taylor, walters |
| Version: | unspecified | Keywords: | patch |
| Hardware: | All | ||
| OS: | All | ||
| URL: | http://cgit.freedesktop.org/~smcv/dbus-glib/log/?h=test-main-context-23633 | ||
| Whiteboard: | |||
| i915 platform: | i915 features: | ||
| Attachments: |
allows API user to switch to non-default main loop context
dbus_g_bus_get_private creates private connection and attaches to specified mainloop context Add a feature test for fd.o #23633, non-default main context Add a feature test for fd.o #23633, non-default main context |
||
I believe the call to dbus_connection_set_timeout_functions has the same bug.
New suggested patch:
diff -urN dbus-glib-orig/dbus/dbus-gmain.c dbus-glib-patched/dbus/dbus-gmain.c
--- dbus-glib-orig/dbus/dbus-gmain.c 2009-09-01 17:09:06.919358522 -0400
+++ dbus-glib-patched/dbus/dbus-gmain.c 2009-09-01 18:32:46.203038866 -0400
@@ -591,14 +591,14 @@
if (!dbus_connection_set_watch_functions (connection,
add_watch,
- remove_watch,
+ NULL,
watch_toggled,
cs, NULL))
goto nomem;
if (!dbus_connection_set_timeout_functions (connection,
add_timeout,
- remove_timeout,
+ NULL,
timeout_toggled,
cs, NULL))
goto nomem;
Created attachment 29259 [details] [review] allows API user to switch to non-default main loop context Hmm, OK without investigating in depth here, my first take would be that you really want an API that allows you to get a private connection already set up with your mainloop. What about a patch that builds on the just committed bug 19623 and adds dbus_g_bus_get_private_with_mainloop or something? (In reply to comment #3) > Hmm, OK without investigating in depth here, my first take would be that you > really want an API that allows you to get a private connection already set up > with your mainloop. > > What about a patch that builds on the just committed bug 19623 and adds > dbus_g_bus_get_private_with_mainloop or something? Or since the just committed patch hasn't been released yet, we could change it to take a mainloop. Created attachment 29725 [details] [review] dbus_g_bus_get_private creates private connection and attaches to specified mainloop context I modified the patch to take in a GMainContext* parameter. While we still can't change contexts after the connection is setup, this patch is a minimal change and provides a way to setup private connections with a specified thread context. (Sorry for the slow reply) I'd like a patch that at least adds something to tests/name-test/test-dbus-glib.c. Colin applied the equivalent of this patch in 0.84, but there still wasn't a complete test. Here's one. Created attachment 45864 [details] [review] Add a feature test for fd.o #23633, non-default main context Created attachment 68927 [details] [review] Add a feature test for fd.o #23633, non-default main context Original patch was good but forgot the test/core/run-test.sh bits. Updated and rebased patch attached. Comment on attachment 68927 [details] [review] Add a feature test for fd.o #23633, non-default main context Review of attachment 68927 [details] [review]: ----------------------------------------------------------------- This is complete nitpicking but when two developers have contributed something, I prefer one of these forms: From: Bob <bob@example.com> Subject: do something Based on a patch from Alice. or From: Alice <alice@example.com> Subject: do something [fixed indentation -Bob] Comment on attachment 68927 [details] [review] Add a feature test for fd.o #23633, non-default main context Review of attachment 68927 [details] [review]: ----------------------------------------------------------------- I'll --amend and apply this. ::: test/core/private.c @@ +1,1 @@ > +/* Feature test for freedesktop.org #23633 - non-default main context You looked at this and didn't object, so I assume my code is OK :-) ::: test/core/run-test.sh @@ +49,4 @@ > ${DBUS_TOP_BUILDDIR}/libtool --mode=execute $DEBUG $DBUS_TOP_BUILDDIR/test/core/test-variant-recursion || die "test-variant-recursion failed" > ${DBUS_TOP_BUILDDIR}/libtool --mode=execute $DEBUG $DBUS_TOP_BUILDDIR/test/core/test-gvariant || die "test-gvariant failed" > ${DBUS_TOP_BUILDDIR}/libtool --mode=execute $DEBUG $DBUS_TOP_BUILDDIR/test/core/test-30574 || die "test-30574 failed" > + ${DBUS_TOP_BUILDDIR}/libtool --mode=execute $DEBUG $DBUS_TOP_BUILDDIR/test/core/test-private || die "test-private failed" Yes, fine. Sorry I forgot this! (In reply to comment #10) > Comment on attachment 68927 [details] [review] [review] > Add a feature test for fd.o #23633, non-default main context > > Review of attachment 68927 [details] [review] [review]: > ----------------------------------------------------------------- > > This is complete nitpicking but when two developers have contributed > something, I prefer one of these forms: > > From: Bob <bob@example.com> > Subject: do something > > Based on a patch from Alice. > > or > > From: Alice <alice@example.com> > Subject: do something > > [fixed indentation -Bob] I hadn't considered my changes significant enough to add my name to the list. I'm fine with your credit. (In reply to comment #11) > Comment on attachment 68927 [details] [review] [review] > Add a feature test for fd.o #23633, non-default main context > > Review of attachment 68927 [details] [review] [review]: > ----------------------------------------------------------------- > > I'll --amend and apply this. > > ::: test/core/private.c > @@ +1,1 @@ > > +/* Feature test for freedesktop.org #23633 - non-default main context > > You looked at this and didn't object, so I assume my code is OK :-) Yes, that seemed fine to me. Reworded to credit you and pushed. Fixed in git for 0.102, thanks. |
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.
Dbus-Glib has a bug where it doesn't process events to a non-default main loop context. e.g. (does not work) DBusGConnection *bus; GError *error = NULL; GMainContext *context = g_main_context_new(); GMainLoop *mainloop = g_main_loop_new (context, FALSE); bus = dbus_g_bus_get(DBUS_BUS_SESSION, &error); dbus_connection_setup_with_g_main (dbus_g_connection_get_connection(bus), context); -------------- I looked into this problem and this is what I found out (in dbus/dbus-gmain.c): When DBus-GLib switches to a new context, it calls add_watch() to attach an IOHandler to poll the file descriptor used for communication. Immediately afterward, DBus invokes DBus-GLib's remove_watch() function, which removes the new IOHandler, instead of the one corresponding to the previous context. Also, the previous IOHandler should be already freed the second time connection_setup_add_watch is called (line 277: dbus_watch_set_data (watch, handler, io_handler_watch_freed) ). I believe remove_watch() should not be called after add_watch(). ---------------- Suggested Patch: diff -urN dbus-glib-orig/dbus/dbus-gmain.c dbus-glib-patched/dbus/dbus-gmain.c --- dbus-glib-orig/dbus/dbus-gmain.c 2009-09-01 17:09:06.919358522 -0400 +++ dbus-glib-patched/dbus/dbus-gmain.c 2009-09-01 17:25:41.237368327 -0400 @@ -591,7 +591,7 @@ if (!dbus_connection_set_watch_functions (connection, add_watch, - remove_watch, + NULL, watch_toggled, cs, NULL)) goto nomem;