[Spice-devel] [spice-gtk PATCH v3 1/5] giopipe: don't fail on create_source

[Marc-André Lureau]

Hi

On Tue, May 26, 2015 at 3:35 PM, Victor Toso <victortoso at redhat.com> wrote:

> PipeInputStream and PipeOutputStream should not fail when creating
> GPollableStream source. It is already checked and unref in case of
> existing source.
>
> In order to track possible issues, the g_return_val_if_fail was
> changed to a g_debug message;
> ---
>  gtk/giopipe.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/gtk/giopipe.c b/gtk/giopipe.c
> index 50edb5b..86eaab6 100644
> --- a/gtk/giopipe.c
> +++ b/gtk/giopipe.c
> @@ -234,10 +234,11 @@ pipe_input_stream_create_source
> (GPollableInputStream *stream,
>      PipeInputStream *self = PIPE_INPUT_STREAM(stream);
>      GSource *pollable_source;
>
> -    g_return_val_if_fail (self->source == NULL ||
> -                          g_source_is_destroyed (self->source), NULL);
> +    if (self->source != NULL && !g_source_is_destroyed (self->source))
> +        g_debug ("%s: GPollableSource already exists %p - This could lead
> to data loss (%ld)",
> +                 G_STRFUNC, self->source, self->read);
>
> -    if (self->source && g_source_is_destroyed (self->source))
> +    if (self->source)
>          g_source_unref (self->source);
>
>      pollable_source = g_pollable_source_new_full (self, NULL,
> cancellable);
> @@ -416,10 +417,11 @@ pipe_output_stream_create_source
> (GPollableOutputStream *stream,
>      PipeOutputStream *self = PIPE_OUTPUT_STREAM(stream);
>      GSource *pollable_source;
>
> -    g_return_val_if_fail (self->source == NULL ||
> -                          g_source_is_destroyed (self->source), NULL);
> +    if (self->source != NULL && !g_source_is_destroyed (self->source))
> +        g_debug ("%s: GPollableSource already exists %p - This could lead
> to data loss (%ld)",
> +                 G_STRFUNC, self->source, self->count);
>
> -    if (self->source && g_source_is_destroyed (self->source))
> +    if (self->source)
>          g_source_unref (self->source);
>
>      pollable_source = g_pollable_source_new_full (self, NULL,
> cancellable);
> --
>

Your tests show that io stream prevents concurrent read / write, and that
self->source is going to be destroyed after the current tasks return.
However, if gpollable create_source is called seperately, it may create
"zombie" sources, they will never be dispatched. It is easy to show the
issue with a test like:

    for (i = 0; i < 10000; i++) {
        GSource *s = g_pollable_input_stream_create_source(f->ip1, NULL);

        g_source_set_callback (s, NULL, NULL, NULL);
        g_source_attach (s, NULL);

        g_source_unref(s);
    }

    g_main_loop_run (f->loop);

After the source is attached, the context will keep a reference, but when
creating a new source, it is not removed from the context. giopipe could
also call g_source_destroy() before the unref, but I don't think this is a
good practice: destroyed source callbacks would never be called, and it may
be hard to understand why.

I suppose the best would be to mimic poll() behaviour, and "dispatch" with
set_ready_time(0) all the created sources (that are still active).

-- 
Marc-André Lureau
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/spice-devel/attachments/20150529/90a8e70d/attachment.html>