Bug 18972

This patch looks OK on a quick first pass.

* I like that we're breaking up the titanic gobject_message_function function a bit.
* Would be good to have a test case.

Will look in more detail a bit later.

Created attachment 21313 [details] [review]
cvs diff -u -N

The original patch introduces a bug: A pointer to the local "Gclosure closure" variable is submitted to the marshaller function which is called in a thread in case of synchronous threaded method calls.

So now the following lines of code in dbus-gobject.c, invoke_object_method():

  GClosure closure;
  . . . . . . . . .
  /* This is evil.  We do this to work around the fact that
   * the generated glib marshallers check a flag in the closure object
   * which we don't care about.  We don't need/want to create
   * a new closure for each invocation.
   */
  memset (&closure, 0, sizeof (closure));


are changed to:

  /* This is evil.  We do this to work around the fact that
   * the generated glib marshallers check a flag in the closure object
   * which we don't care about.  We don't need/want to create
   * a new closure for each invocation.
   */
  static GClosure closure = {0};

Do we need to expose dbus_g_thread_pool_init as public API?  I'm not a big fan of required global initialization functions.  

If we do need to expose it, can we associate it with a DBusGConnection?  For example, dbus_g_connection_set_max_threads.  We should also allocate the pool when registering an object that has the annotation.

Created attachment 22123 [details] [review]
cvs diff -u -N

Initialization of the thread pool is now hidden.

Do you think you might be able to rebase this patch on git master?  There were changes that just landed for http://bugs.freedesktop.org/show_bug.cgi?id=19441 

Created attachment 22512 [details] [review]
git diff -u

patch was rebased to git master

Couple of quick things:

+  DBusTask *task = g_slice_new (DBusTask);
+  if (NULL == task)
+    {
+      g_error ("Could not create memory slice");
+      return NULL;
+    }

As far as I know g_slice_new shares the semantics of g_malloc; i.e. it can't fail (well, aborts on fail).

+gboolean
+_dbus_gasync_thread_pool_start (GThreadedFunc callback,
+                                gpointer      data);

Functions like this should be annotated G_GNUC_INTERNAL.

@@ -2379,11 +2627,11 @@ void
 dbus_g_method_return_error (DBusGMethodInvocation *context, GError *error)
 {
   DBusMessage *reply;
-  
+

I'd prefer no spurious whitespace changes; I know trailing whitespace sucks, but better to fix it incrementally.

+      if (send_reply &&
+          !add_output_arguments (&iter,
+                                 object_info,
+                                 method,
+                                 out_param_values,
+                                 out_param_gvalues))

If I understand the new code correctly, add_output_arguments also has the responsibility to free any memory given to us by the invokee.  So in the case where we're not sending a reply, we'd leak memory I believe.  If this is the case we should probably add a flag to add_output_arguments which tells it to append (or maybe a NULL iter signifies this).

That's what I saw in a first pass, it's a complex patch for a really hairy bit of code, but it looks like it's for the better.  

The most important thing here is to get some tests in; I'll take a look at doing that.

Random thought - what if we made the annotation

      <annotation name="org.freedesktop.DBus.GLib.Async" value="thread"/>

?

Created attachment 27766 [details] [review]
patch in git format-patch, with test cases

This patch is in git am format, and adds some test cases.

While writing the test cases, I don't understand why we create a DBusGMethodInvocation in the SYNCHRONOUS_THREADED case.  The intent of that argument is to have something to use for return values in the ASYNCHRONOUS case, but in SYNCHRONOUS_THREADED case the calling convention we're using is the same as SYNCHRONOUS where return values are given to the function directly.

Sorry again about taking so long to process this patch.  I think given the outstanding issues we should move it to the release after 0.82, but make sure we get it in for 0.84.

(In reply to comment #9)
> Created an attachment (id=27766) [details]
> patch in git format-patch, with test cases
> 
> This patch is in git am format, and adds some test cases.
> 
> While writing the test cases, I don't understand why we create a
> DBusGMethodInvocation in the SYNCHRONOUS_THREADED case.  The intent of that
> argument is to have something to use for return values in the ASYNCHRONOUS
> case, but in SYNCHRONOUS_THREADED case the calling convention we're using is
> the same as SYNCHRONOUS where return values are given to the function directly.

  struct _DBusGMethodInvocation {
    DBusGConnection *connection; /**< The connection */
    DBusGMessage *message; /**< The message which generated the method call */
    const DBusGObjectInfo *object; /**< The object the method was called on */
    const DBusGMethodInfo *method; /**< The method called */
    gboolean send_reply;
  };

  /*
   * Append DBusGMethodInvocation as final argument in case of
   * asynchronous or synchronous threaded calls
   */
  if (ASYNCHRONOUS==call_type || SYNCHRONOUS_THREADED==call_type)
    {
      GValue context_value = {0,};
      DBusGMethodInvocation *context;
      context = g_new (DBusGMethodInvocation, 1);
      context->connection = dbus_g_connection_ref (DBUS_G_CONNECTION_FROM_CONNECTION (connection));
      context->message = dbus_g_message_ref (DBUS_G_MESSAGE_FROM_MESSAGE (message));
      context->object = object_info;
      context->method = method;
      context->send_reply = send_reply;
      . . .

Decided to use the DBusGMethodInvocation since we need all the data in the
new thread when sending back response to the client. invoke_marshaller_threaded ()
needs the message to create the response and the object and the method
to properly add the output arguments. The connection is then used for sending
back the response to the client. All that stuff is done for the SYNCHRONOUS
case as well in invoke_object_method () but with the only difference that
invoke_object_method () already takes those as arguments.

I could provide all that data to invoke_marshaller_threaded () by extending
DBusGSyncThreadedData instead as shown below but i thought the solution above
was cleaner.

      method_data = g_slice_new (DBusGSyncThreadedData);

      method_data->method             = method;
      method_data->closure            = &closure;
      method_data->value_array        = value_array;
      method_data->out_param_values   = out_param_values;
      method_data->out_param_gvalues  = out_param_gvalues;
     +
     +method_data->connection = ...
     +method_data->message    = ...
     +method_data->object     = ...
     +method_data->method     = ...

      if (!_dbus_gasync_thread_pool_start (invoke_marshaller_threaded,
                                           method_data))
      . . .

Please advise if you prefer it done in another way.

> Sorry again about taking so long to process this patch.  I think given the
> outstanding issues we should move it to the release after 0.82, but make sure
> we get it in for 0.84.
> 

GDBus has a considerably better record on thread-safety and is provided by one of our dependencies, so I'd be tempted to WONTFIX this?

Last substantial comment was from 2009.  Closing it.  It can be reopened if the author thinks it is still relevant.

I know it is now almost 3 years after the ticket was closed but what do you think about including the patch upstream anyway? I am still thinking that the threaded feature is quite useful in case one does not want to block the main loop of the application.
I am not quite sure whether GDBus has completely taken over now, i am personally still using dbus-glib and this particular feature.
I was not active for quite a while now so please close the ticket if you think the patch is not relevant anymore.

(In reply to comment #13)
> I know it is now almost 3 years after the ticket was closed but what do you
> think about including the patch upstream anyway? I am still thinking that
> the threaded feature is quite useful in case one does not want to block the
> main loop of the application.

dbus-glib is not thread-safe, and its design makes it difficult to be thread-safe. If synchronous method calls in a thread pool work, they work by coincidence.

I strongly recommend GDBus, especially if you want to play nicely with threads.