Re: GDBus socket code ...

[David Zeuthen <zeuthen gmail com>]

Hi,

(Wouldn't it have been better to file this in bugzilla?)

On Wed, Aug 11, 2010 at 7:52 AM, Michael Meeks <michael meeks novell com> wrote:
> Hi there,
>
>        After the mythically curious dbus socket code [ as Havoc says
> ~"re-writing it would be easier than understanding it" ] - I was full of
> enthusiasm for the new gdbus.
>
>        The new code, while more readable and promising, seems to have a few
> drop-offs. The first I noticed was getting a raise (SIGABRT); when a
> socket buffer is full, and sendmsg fails with EAGAIN (as seen in e-d-s).
>
>        Sadly under strace the timings change and I no longer get the EAGAIN to
> prove it (sad but true), but with this:
>
> diff --git a/gio/gdbusprivate.c b/gio/gdbusprivate.c
> index cd6eb91..c0b445d 100644
> --- a/gio/gdbusprivate.c
> +++ b/gio/gdbusprivate.c
> @@ -997,6 +997,8 @@ write_message_in_idle_cb (gpointer user_data)
>                           data,
>                           &error))
>         {
> +         if (error && error->code == G_IO_ERROR_WOULD_BLOCK)
> +               g_error ("crash and burn on EAGAIN\n");
>           /* TODO: handle */
>           _g_dbus_worker_emit_disconnected (worker, TRUE, error);
>           g_error_free (error);
>
>        I can get a nice console g_error of the same type before I get the
> SIGTERM ;-) Since e-d-s shunts really quite a lot of data over the bus
> this is rather a common case.
>
>        In historic types in ORBit2 / linc - we had a custom GSource (which
> IMHO is well worth stealing), for which we could strobe the poll
> conditions easily [ though perhaps in glib that is easier anyway ].
>
>        Thus - when we hit an EAGAIN on write, we would immediately switch our
> poll to wait until there is space on the socket buffer (G_IO_OUT), and
> get on with processing any other data / backlog and/or sleep.
>
>        It -seems- as if the approach in gdbus (as of now) is to have an idle
> handler running in the I/O thread [ the I/O thread model is a good one
> IMHO ], that will rapidly turn into a busy loop - simply re-dispatching
> the idle handler indefinitely [ assuming we don't fail on EAGAIN ;-]
> until we can write the data.
>
>        Is that the ultimately intention ? or is this just somewhat
> under-finished ?

Right now, the intent is to block the IO thread until all data has been written.

Anyway, the GSocket for sure is in non-blocking mode (see
gdbusconnection.c's initable_init() function) so getting
G_IO_ERROR_WOULD_BLOCK can happen. And we clearly don't handle that.

Anyway, I wonder if it's enough to just switch the socket into
blocking mode. Can you try that? (I think this is safe to do this
because we have our own reading routines in gdbusprivate.c,
_g_socket_read_with_control_messages(), that doesn't care if the
socket is in blocking mode or not...)


>        I was also rather surprised to get this SIGTERM behaviour since all
> calls to set_exit_on_close pass FALSE that I can see anywhere ;-)

Can't say where the SIGTERM is coming from but note that GDBus will
scribble on stdout before raising SIGTERM, see

 http://git.gnome.org/browse/glib/tree/gio/gdbusconnection.c#n544

My guess is that GDBus is not to blame here.

     David