>
> Hi again,
>
> On Mon, Jul 1, 2019 at 3:16 PM Jakub Janku <jjanku@xxxxxxxxxx> wrote:
> >
> > Hi,
> >
> > On Mon, Jul 1, 2019 at 1:02 PM Frediano Ziglio <fziglio@xxxxxxxxxx> wrote:
> > >
> > > >
> > > > The current approach with OutputQueue in webdav has several problems:
> > > >
> > > > * if the connection is slow, webdav keeps reading from phodav
> > > > and pushing messages to the internal channel xmit_queue.
> > > > This way, the queue can grow very quickly and the whole file
> > > > that is being transferred using webdav essentially gets loaded into
> > > > memory.
> > > >
> > > > * spice channel first flushes all messages in the xmit_queue and
> > > > then proceeds to reading. If webdav floods the xmit_queue with
> > > > a ton of messages, spice channel does not leave iterate_write until
> > > > the queue gets empty. This way, reading from the channel is blocked
> > > > till the whole file is transferred.
> > > >
> > > > * OutputQueue uses g_output_stream_flush_async() on
> > > > SpiceVmcOutputStream
> > > > that does not implement flush
> > > >
> > > > To solve these issues, don't read from phodav until the last message
> > > > for a given client is written out to the socket.
> > > > (main channel currently uses the same approach when transferring files)
> > > >
> > > > OutputQueue used an idle function to schedule the write and then
> > > > called mux_pushed_cb which started reading from phodav with priority
> > > > G_PRIORITY_DEFAULT.
> > > > Since this new approach does not utilize the idle scheduling,
> > > > lower the priority in client_start_read() to G_PRIORITY_DEFAULT_IDLE
> > > > to make sure webdav does not block other channels.
> > > >
> > >
> > > This sounds a bit worrying. Why with all coroutine/async code we
> > > have we should have a blockage? This sounds wrong.
> >
> > To be honest, I did not spent much time looking at why this blocks.
> > Lowering the priority of webdav makes sense to me regardless of this issue.
> > Another thing is that with your spice-server series concerning
> > spicevmc and DoS, this isn't an issue.
>
> I did some more testing and here's what I've found out:
>
> Channel display uses g_coroutine_signal_emit() to emit the
> "display-invalidate" signal.
> This function uses an idle function (G_PRIORITY_DEFAULT_IDLE = 200) to
> emit the signal in the main context.
>
> From Gtk docs (G_PRIORITY_HIGH_IDLE = 100; larger number means lower
> priority):
> "GTK+ uses G_PRIORITY_HIGH_IDLE + 10 for resizing operations, and
> G_PRIORITY_HIGH_IDLE + 20 for redrawing operations."
>
> This means that if the client_start_read() in webdav uses
> G_PRIORITY_DEFAULT= 0, it can block these two operations rather easily
> causing the widget not to redraw. But as I said, this shouldn't be an
> issue if the channel was throttled down by the server.
>
> So is it ok to lower the priority in webdav to G_PRIORITY_DEFAULT_IDLE?
>
It sounds a good reason. Maybe copy this explanation too.
Saying just "block other channels" with all this code seems to suggest more
a synchonous code which will block than a priority issue. Also I think that
this patch should at least reduce the blocking in this case.
By the way, I found some time to test my patch and update your patch to
use asynchronous writing to the channel in order to remove the other
potential blockage.
> Cheers,
> Jakub
> > >
> > > > Also implement spice_webdav_channel_reset(). This is necessary because
> > > > spice_vmc_write_async() references the channel. If the channel is to be
> > > > disconnected, the write operations need to be cancelled so that
> > > > the references to the channel are released asap. Otherwise,
> > > > spice session would be stuck waiting for the channel to finalize.
> > > >
> > > > Signed-off-by: Jakub Janků <jjanku@xxxxxxxxxx>
> > > > ---
> > > >
> > > > Note: I left the OutputQueue code still in place since I'm considering
> > > > using it for the drag&drop functionality.
> > > >
> > > > src/channel-webdav.c | 49 ++++++++++++++++++++++++++++++++------------
> > > > 1 file changed, 36 insertions(+), 13 deletions(-)
> > > >
> > > > diff --git a/src/channel-webdav.c b/src/channel-webdav.c
> > > > index f5a38ad..66f8a5b 100644
> > > > --- a/src/channel-webdav.c
> > > > +++ b/src/channel-webdav.c
> > > > @@ -45,13 +45,12 @@
> > > > * Since: 0.24
> > > > */
> > > >
> > > > -typedef struct _OutputQueue OutputQueue;
> > > > +/* typedef struct _OutputQueue OutputQueue; */
> > > >
> > > > struct _SpiceWebdavChannelPrivate {
> > > > SpiceVmcStream *stream;
> > > > GCancellable *cancellable;
> > > > GHashTable *clients;
> > > > - OutputQueue *queue;
> > > >
> > > > gboolean demuxing;
> > > > struct _demux {
> > > > @@ -65,6 +64,7 @@ G_DEFINE_TYPE_WITH_PRIVATE(SpiceWebdavChannel,
> > > > spice_webdav_channel, SPICE_TYPE_
> > > >
> > > > static void spice_webdav_handle_msg(SpiceChannel *channel, SpiceMsgIn
> > > > *msg);
> > > >
> > > > +#if 0
> > > > struct _OutputQueue {
> > > > GOutputStream *output;
> > > > gboolean flushing;
> > > > @@ -179,6 +179,8 @@ static void output_queue_push(OutputQueue *q, const
> > > > guint8 *buf, gsize size,
> > > > q->idle_id = g_idle_add(output_queue_idle, q);
> > > > }
> > > >
> > > > +#endif
> > > > +
> > > > typedef struct Client
> > > > {
> > > > guint refs;
> > > > @@ -227,10 +229,17 @@ static void remove_client(Client *client)
> > > > g_hash_table_remove(client->self->priv->clients, &client->id);
> > > > }
> > > >
> > > > -static void mux_pushed_cb(OutputQueue *q, gpointer user_data)
> > > > +#define MAX_MUX_SIZE G_MAXUINT16
> > > > +
> > > > +static void
> > > > +mux_msg_flushed_cb(GObject *source_object,
> > > > + GAsyncResult *result,
> > > > + gpointer user_data)
> > > > {
> > > > Client *client = user_data;
> > > >
> > > > + g_output_stream_write_finish(G_OUTPUT_STREAM(source_object),
> > > > result,
> > > > NULL);
> > > > +
> > > > if (client->mux.size == 0 ||
> > > > !client_start_read(client)) {
> > > > remove_client(client);
> > > > @@ -239,14 +248,13 @@ static void mux_pushed_cb(OutputQueue *q,
> > > > gpointer
> > > > user_data)
> > > > client_unref(client);
> > > > }
> > > >
> > > > -#define MAX_MUX_SIZE G_MAXUINT16
> > > > -
> > > > static void server_reply_cb(GObject *source_object,
> > > > GAsyncResult *res,
> > > > gpointer user_data)
> > > > {
> > > > Client *client = user_data;
> > > > SpiceWebdavChannelPrivate *c = client->self->priv;
> > > > + GOutputStream *mux_out;
> > > > GError *err = NULL;
> > > > gssize size;
> > > >
> > > > @@ -258,10 +266,15 @@ static void server_reply_cb(GObject
> > > > *source_object,
> > > > g_return_if_fail(size >= 0);
> > > > client->mux.size = size;
> > > >
> > > > - output_queue_push(c->queue, (guint8 *)&client->mux.id,
> > > > sizeof(gint64),
> > > > NULL, NULL);
> > > > + mux_out = g_io_stream_get_output_stream(G_IO_STREAM(c->stream));
> > > > +
> > > > + g_output_stream_write(mux_out, (guint8 *)&client->mux.id,
> > > > sizeof(gint64), NULL, NULL);
> > > > client->mux.size = GUINT16_TO_LE(client->mux.size);
> > > > - output_queue_push(c->queue, (guint8 *)&client->mux.size,
> > > > sizeof(guint16), NULL, NULL);
> > > > - output_queue_push(c->queue, (guint8 *)client->mux.buf, size,
> > > > (GFunc)mux_pushed_cb, client);
> > > > + g_output_stream_write(mux_out, (guint8 *)&client->mux.size,
> > > > sizeof(guint16), NULL, NULL);
> > > > + /* 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_async(mux_out, (guint8 *)client->mux.buf,
> > > > size,
> > > > + G_PRIORITY_DEFAULT, client->cancellable, mux_msg_flushed_cb,
> > > > client);
> > >
> > > Why don't you use g_output_stream_writev_all_async here writing all 3
> > > buffers
> > > together and avoid blocking calls?
> >
> > Actually, all of these 3 calls are in a way synchronous -- look at
> > SpiceVmcOutputStream implementation,
> > they all create a new out message and append it to the channel's
> > xmit_queue.
> > The only difference is that with the async call, we can provide a
> > function that is called back once the message is written out.
> >
> > I agree that the 3 buffers could be written together, but writing them
> > separately wasn't introduced by me.
> >
> > > Well, it's available starting from GLib 2.60 so possibly you'll have
> > > to provide a replacement for GLib 2.60 using
> > > g_output_stream_write_all_async.
> > >
> > > >
> > > > return;
> > > >
> > > > @@ -284,8 +297,9 @@ static bool client_start_read(Client *client)
> > > > if (g_input_stream_is_closed(input)) {
> > > > return false;
> > > > }
> > > > + /* use low priority to leave enough time for other channels */
> > > > g_input_stream_read_async(input, client->mux.buf, MAX_MUX_SIZE,
> > > > - G_PRIORITY_DEFAULT, client->cancellable,
> > > > server_reply_cb,
> > > > + G_PRIORITY_DEFAULT_IDLE,
> > > > client->cancellable,
> > > > server_reply_cb,
> > > > client_ref(client));
> > > > return true;
> > > > }
> > > > @@ -540,9 +554,6 @@ static void
> > > > spice_webdav_channel_init(SpiceWebdavChannel
> > > > *channel)
> > > > c->clients = g_hash_table_new_full(g_int64_hash, g_int64_equal,
> > > > NULL, client_remove_unref);
> > > > c->demux.buf = g_malloc0(MAX_MUX_SIZE);
> > > > -
> > > > - GOutputStream *ostream =
> > > > g_io_stream_get_output_stream(G_IO_STREAM(c->stream));
> > > > - c->queue = output_queue_new(ostream);
> > > > }
> > > >
> > > > static void spice_webdav_channel_finalize(GObject *object)
> > > > @@ -560,7 +571,6 @@ static void spice_webdav_channel_dispose(GObject
> > > > *object)
> > > >
> > > > g_cancellable_cancel(c->cancellable);
> > > > g_clear_object(&c->cancellable);
> > > > - g_clear_pointer(&c->queue, output_queue_free);
> > > > g_clear_object(&c->stream);
> > > > g_hash_table_unref(c->clients);
> > > >
> > > > @@ -572,6 +582,18 @@ static void spice_webdav_channel_up(SpiceChannel
> > > > *channel)
> > > > CHANNEL_DEBUG(channel, "up");
> > > > }
> > > >
> > > > +static void spice_webdav_channel_reset(SpiceChannel *channel, gboolean
> > > > migrating)
> > > > +{
> > > > + SpiceWebdavChannelPrivate *c;
> > > > + c =
> > > > spice_webdav_channel_get_instance_private(SPICE_WEBDAV_CHANNEL(channel));
> > > > +
> > > > + g_cancellable_cancel(c->cancellable);
> > > > + c->demuxing = FALSE;
> > > > + g_hash_table_remove_all(c->clients);
> > > > +
> > > > +
> > > > SPICE_CHANNEL_CLASS(spice_webdav_channel_parent_class)->channel_reset(channel,
> > > > migrating);
> > > > +}
> > > > +
> > > > static void spice_webdav_channel_class_init(SpiceWebdavChannelClass
> > > > *klass)
> > > > {
> > > > GObjectClass *gobject_class = G_OBJECT_CLASS(klass);
> > > > @@ -581,6 +603,7 @@ static void
> > > > spice_webdav_channel_class_init(SpiceWebdavChannelClass *klass)
> > > > gobject_class->finalize = spice_webdav_channel_finalize;
> > > > channel_class->handle_msg = spice_webdav_handle_msg;
> > > > channel_class->channel_up = spice_webdav_channel_up;
> > > > + channel_class->channel_reset = spice_webdav_channel_reset;
> > > >
> > > > g_signal_override_class_handler("port-event",
> > > > SPICE_TYPE_WEBDAV_CHANNEL,
> > >
> > > Frediano
> >
> > Thanks,
> > Jakub
>
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel