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

[Victor Toso]

Hi,

On Sat, Aug 24, 2019 at 10:39:03AM +0200, Jakub Janku wrote:
> 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.

Right. I think vmcstream's implementation was not just targeting
spice+webdav use case but trying to be a bit more generic.

I don't see our code changes as a problem, as long as webdav
keeps moving on. Thanks for the patches!

I'll be pushing shortly.

Cheers,

> > 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.

Acked-by: Victor Toso <victortoso at redhat.com>

> 
> 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20190826/ef620b2f/attachment.sig>