[Spice-devel] [PATCH spice-gtk] webdav: fix usecase with multiple concurrent connections

[Jakub Janku]

Hi,

On Fri, Aug 23, 2019 at 3:36 PM Victor Toso <victortoso at redhat.com> wrote:
>
> Hi,
>
> On Fri, Aug 23, 2019 at 11:57:55AM +0200, Jakub Janku wrote:
> > ping
>
> Yep, sorry
>
> >
> > Also forgot to mention that this fixes a regression introduced by me
> > in 9f5aee05.
>
> Ok. I just went over it before coming back here.
>
> > On Thu, Aug 8, 2019 at 11:03 AM Jakub Janků <jjanku at redhat.com> wrote:
> > >
> > > GOutputStream does not allow simultaneous tasks on a single
> > > stream. An attempt to transfer two files therefore
> > > results into one of the clients being removed in
> > > mux_msg_flushed_cb() with the error
> > > "Stream has outstanding operation".
> > >
> > > To fix this, use spice_vmc_write_async() directly.
>
> The major difference in implementation that this patch proposes
> is to avoid a GTask creation and being handled by vmcstream.c on
> spice_vmc_output_stream_write_async(), correct?

That's correct.
Before this patch: g_output_stream_write_all_async() ->
spice_vmc_output_stream_write_async() -> spice_vmc_write_async()
With this patch, spice_vmc_write_async() is called directly, without
the first 2 steps.
>
> I'm a bit confused about that, maybe because it was working
> before? I mean, the fact that we changed the way to deal with
> the buffers made our implementation on GOutputStream not
> support simultaneous tasks or that should never actually work in
> the first place?

Previously, webdav channel had its own OutputQueue that was scheduling
the messages and calling g_output_stream_write_all() for each message.
With 9f5aee05, the OutputQueue was dropped and webdav now relies on
the internal queue in SpiceChannel.
Maybe it's the naming that might confuse you. "spice_vmc_write_async"
might suggest that the message is simply written to the stream, but
the message is in reality queued for write and the async operation
finishes once the msg is flushed. Therefore you can call
spice_vmc_write_async() multiple times in a row, unlike
g_output_stream_write_async() that permits only a single operation at
a time.
>
> I have to refresh my memory on the implementation details on this
> but the concept of pipe per 'client' could work with multiple
> files because of the inner protocol that mux/demux things on
> client/server, but perhaps I'm misremembering.
>
> So, again, my question would be: Did the overall behavior
> changed? e.g: This client still works with old spice-webdavd for
> instance.

Yes, it works with old spice-webdavd versions.

Cheers,
Jakub
>
> Thanks,
>
> > > Signed-off-by: Jakub Janků <jjanku at redhat.com>
> > > ---
> > >  src/channel-webdav.c | 17 +++++++----------
> > >  1 file changed, 7 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/src/channel-webdav.c b/src/channel-webdav.c
> > > index 14d4e05..09ef9f7 100644
> > > --- a/src/channel-webdav.c
> > > +++ b/src/channel-webdav.c
> > > @@ -235,7 +235,7 @@ mux_msg_flushed_cb(GObject *source_object,
> > >  {
> > >      Client *client = user_data;
> > >
> > > -    if (!g_output_stream_write_all_finish(G_OUTPUT_STREAM(source_object), result, NULL, NULL) ||
> > > +    if (spice_vmc_write_finish(SPICE_CHANNEL(source_object), result, NULL) == -1 ||
> > >          client->mux.size == 0 ||
> > >          !client_start_read(client)) {
> > >          remove_client(client);
> > > @@ -249,8 +249,6 @@ static void server_reply_cb(GObject *source_object,
> > >                              gpointer user_data)
> > >  {
> > >      Client *client = user_data;
> > > -    SpiceWebdavChannelPrivate *c = client->self->priv;
> > > -    GOutputStream *mux_out;
> > >      GError *err = NULL;
> > >      gssize size;
> > >
> > > @@ -262,13 +260,12 @@ static void server_reply_cb(GObject *source_object,
> > >      g_return_if_fail(size >= 0);
> > >      client->mux.size = GUINT16_TO_LE(size);
> > >
> > > -    mux_out = g_io_stream_get_output_stream(G_IO_STREAM(c->stream));
> > > -
> > > -    /* this internally uses spice_vmc_write_async(), priority is ignored;
> > > -     * the callback is invoked once the msg is written out to the socket */
> > > -    g_output_stream_write_all_async(mux_out, (guint8 *)&client->mux, sizeof(gint64) + sizeof(guint16) + size,
> > > -        G_PRIORITY_DEFAULT, client->cancellable, mux_msg_flushed_cb, client);
> > > -
> > > +    spice_vmc_write_async(SPICE_CHANNEL(client->self),
> > > +                          &client->mux,
> > > +                          sizeof(gint64) + sizeof(guint16) + size,
> > > +                          client->cancellable,
> > > +                          mux_msg_flushed_cb,
> > > +                          client);
> > >      return;
> > >
> > >  end:
> > > --
> > > 2.21.0
> > >
> > _______________________________________________
> > Spice-devel mailing list
> > Spice-devel at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/spice-devel