[Spice-devel] [spice-gtk v3 05/16] file-xfer: inform agent of errors only when task finished

[Jonathon Jongsma]

On Tue, 2016-06-07 at 11:14 -0500, Jonathon Jongsma wrote:
> On Sun, 2016-06-05 at 10:38 +0200, Victor Toso wrote:
> > 
> > Hi,
> > 
> > On Fri, Jun 03, 2016 at 10:17:05AM -0500, Jonathon Jongsma wrote:
> > > 
> > > 
> > > Related to my comments to the last patch, this change makes the
> > > SpiceFileTransferTask class less "self-contained". In other words, if
> > > the user does not connect to the 'finished' signal and send the
> > > appropriate agent XFER_STATUS message in that handler, the file
> > > transfer won't work properly.
> > I don't follow.
> > 
> > Although the user/application can attach to SpiceFileTransferTask'
> > signals, we definitely don't rely on it.
> > 
> > Channel-main connects to the 'finished' and once that is emitted we will
> > catch that on task_finished() which should inform the agent the error.
> > That removes the need of SpiceFileTransferTask to send error to the
> > agent.
> > 
> > The user/application can either know the error from 'finished' or from
> > the ending callback call which, at the time of this patch, still comes
> > from SpiceFileTransferTask which means that g_task_return_error will be
> > called when the stream is closed at file_xfer_close_cb();
> > 
> > In the end of the series I change that slightly as I don't think that we
> > want to call the user callback from spice_main_file_copy_async() per
> > SpiceFileTransferTask but still, the user/application should be able to
> > get the error without connecting to 'finished'
> Hmm, re-reading my comment, I find it hard to understand as well. Sorry I
> wasn't
> more clear. In essence, I was working from the assumption that the
> SpiceFileTransferTask should be fully in charge of the file transfer. In other
> words, when you call _start(), it should handle all of the file transfer logic
> and send all of the messages and indicate the status back to the MainChannel.
> From that point of view, when the SpiceFileTransferTask emits "finished", the
> transfer should actually be finished. But it is not actually finished yet,
> because we haven't yet sent the VD_AGENT_FILE_XFER_STATUS message in the case
> where there has been an error. 
> 
> But since my assumption above was wrong, the above comment doesn't make much
> sense. Even so, I think that a "finished" signal is perhaps not the best way
> to
> handle this. see below.
> 
> > 
> > 
> > > 
> > > 
> > > I think it's probably more maintainable and less error-prone if the
> > > SpiceFileTransferTask has the responsibility for all of the logic for
> > > conducting the file transfer.
> > I don't really agree, for me it makes sense to have clear the tasks of
> > each object so we can deal the situations more easily. The
> > SpiceFileTransferTask does get the file-info, the data from file,
> > progressbar info, handles Cancellation; Channel-Main is in charge with
> > communication with the agent, flushing data, start/stop/reset
> > file-transfer.
> OK, so I'll accept your division of responsibilities here. But I still think
> that the design could be improved a bit. I have some vague ideas that I'm
> still
> mulling over and will try to present them in a future email after I determine
> whether they're workable or not.



OK, so here's my attempt to flesh out my ideas for the design.

the _start() function could be changed to something like:

spice_file_transfer_task_query_info_async(SpiceFileTransferTask*,
GAsyncReadyCallback, ...)
spice_file_transfer_task_query_info_finish()

One thing that seems a bit out-of-place to me is the "flush_callback" in
SpiceFileTransferTask. with the current design, the SpiceFileTransferTask
doesn't really need to know anything about the concept of "flushing". All it
needs to do is provide a chunk of data to the main channel to send. Then it
should just wait for the main channel to request the next chunk. The main
channel is the only one that should need to know anything about flushing.

So perhaps we could remove the SpiceFileTransferTaskFlushCb callback and simply
replace it with something like the following API (based on
g_file_input_stream_read_async()):

spice_file_transfer_task_read_next_chunk_async(SpiceFileTransferTask*, void*
buffer, gsize count, GAsyncReadyCallback, ...)
gssize spice_file_transfer_task_read_next_chunk_finish()

In other words, we don't pass a flush_callback to the task when we create it;
this callback is instead passed as an argument to _read_next_chunk_async(). And
_read_next_chunk_async() is basically a replacement for
spice_file_transfer_task_flush_done()

[I thought about simply exposing the GFileInputStream to the MainChannel to do
those read operations (rather than providing the wrapper functions mentioned
above), but I think that would leak too much logic back into MainChannel, which
we're trying to avoid.]

I don't know if those ideas help at all. Probably function names could be
improved, etc. And maybe attempting to implement something like that would
expose some problems with the design. Interested in your thoughts.

Jonathon