After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 607620 - Cancelled uploads appear partially complete in PicasaWeb
Cancelled uploads appear partially complete in PicasaWeb
Status: RESOLVED FIXED
Product: libgdata
Classification: Platform
Component: PicasaWeb service
git master
Other All
: Normal normal
: ---
Assigned To: libgdata-maint
libgdata-maint
Depends on:
Blocks:
 
 
Reported: 2010-01-21 02:38 UTC by Richard Schwarting
Modified: 2010-11-27 23:09 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Patches gdata/gdata-upload-stream.c (2.40 KB, patch)
2010-11-25 11:38 UTC, George Stephanos
none Details | Review
Correct patch. Submitted wrong one earlier. (1.43 KB, patch)
2010-11-25 11:54 UTC, George Stephanos
needs-work Details | Review

Description Richard Schwarting 2010-01-21 02:38:44 UTC
With sufficiently large images, if we cancel the upload midway, an entry still appears on PicasaWeb.

Is this sane?  Thinking of parsing the new entry in cases where we get errors during splicing, and then deleting the file.  I could perhaps do it just when the cancellable is cancelled (in case we err for some other, unspecified reason that could actually leave a file successfully uploaded).  I'm not quite sure how to handle potential errors from parse_spliced_stream and gdata_service_delete_entry, since we already have one from g_output_stream_splice.  Can we just prefer that earliest error for propagation?

gdata_picasaweb_service_upload_file
===================================

g_output_stream_splice (output_stream, input_stream,
                        G_OUTPUT_STREAM_SPLICE_CLOSE_SOURCE |
                        G_OUTPUT_STREAM_SPLICE_CLOSE_TARGET,
                        cancellable, &child_error);

g_object_unref (input_stream);


if (child_error != NULL) {
        /* something to parse the response and call delete on it */
        new_entry = parse_spliced_stream (output_stream, error2); /* DBG */
        if (GDATA_PICASAWEB_IS_FILE (new_entry)) {
  	  gdata_service_delete_entry (GDATA_SERVICE (self), 
                                      GDATA_ENTRY (new_entry), NULL, error2); 
        }

        g_object_unref (output_stream);
        g_propagate_error (error, child_error);
        return NULL;
}

new_entry = parse_spliced_stream (output_stream, error);
g_object_unref (output_stream);

return new_entry;

=====================================

Will also check what to do regarding the async version.
Comment 1 Philip Withnall 2010-01-21 14:30:23 UTC
(In reply to comment #0)
> With sufficiently large images, if we cancel the upload midway, an entry still
> appears on PicasaWeb.

Seems odd that the server would accept a partial upload. Can you get a Wireshark trace of the upload traffic, and see if we're doing anything odd to terminate the stream on cancellation? If so, that should be fixed.

> Is this sane?  Thinking of parsing the new entry in cases where we get errors
> during splicing, and then deleting the file.  I could perhaps do it just when
> the cancellable is cancelled (in case we err for some other, unspecified reason
> that could actually leave a file successfully uploaded).  I'm not quite sure
> how to handle potential errors from parse_spliced_stream and
> gdata_service_delete_entry, since we already have one from
> g_output_stream_splice.  Can we just prefer that earliest error for
> propagation?

Propagate the error from g_output_stream_splice(), since that's the one which tells the user about cancellation. The others are just incidental, since we're cleaning up after ourselves. If we can't delete the entry, there's not much the user could do about it.

> gdata_picasaweb_service_upload_file
> ===================================
> 
> g_output_stream_splice (output_stream, input_stream,
>                         G_OUTPUT_STREAM_SPLICE_CLOSE_SOURCE |
>                         G_OUTPUT_STREAM_SPLICE_CLOSE_TARGET,
>                         cancellable, &child_error);
> 
> g_object_unref (input_stream);
> 
> 
> if (child_error != NULL) {
>         /* something to parse the response and call delete on it */
>         new_entry = parse_spliced_stream (output_stream, error2); /* DBG */
>         if (GDATA_PICASAWEB_IS_FILE (new_entry)) {
>         gdata_service_delete_entry (GDATA_SERVICE (self), 
>                                       GDATA_ENTRY (new_entry), NULL, error2); 
>         }
> 
>         g_object_unref (output_stream);
>         g_propagate_error (error, child_error);
>         return NULL;
> }
> 
> new_entry = parse_spliced_stream (output_stream, error);
> g_object_unref (output_stream);
> 
> return new_entry;
> 
> =====================================
> 
> Will also check what to do regarding the async version.

Thanks.
Comment 2 Philip Withnall 2010-10-28 15:11:14 UTC
It would be interesting to see if there's something like upload resume support for PicasaWeb as described in bug #607272.
Comment 3 Richard Schwarting 2010-10-28 18:12:12 UTC
Hello Philip.

Sorry for disappearing for the last 6 months, but I ended up getting a job and started a Masters of Science.  I'm trying to stabilise my life right now to schedule time to resume contributions, though. 

I believe you've balanced school and work fairly well before, and would be glad to take any advice you have on it :)

Cheerio, 
   Richard
Comment 4 George Stephanos 2010-11-25 01:39:30 UTC
I've found the cause of this bug to be a simple footer that's sent right after the image even if the upload was interrupted (when the connection actually has to be terminated without anymore I/O)
As a solution, in gdata/gdata-upload-stream.c, we could somehow export the cancellable in gdata_upload_stream_write() to wrote_chunk_cb() and under the check for reached_eof, we could add a check for the cancellable (if it was triggered). If it was, we can return wrote_chunk_cb() without resuming the upload.
Comment 5 George Stephanos 2010-11-25 01:44:15 UTC
Sorry for not being clear about why the bug happens.
When wrote_chunk_cb() doesn't find anymore data to type, it assumes its an EOF. Which means that it accepts both partially and fully completed data streams blindly. A simple check for the cancellable can be done as said.
Comment 6 George Stephanos 2010-11-25 11:38:13 UTC
Created attachment 175234 [details] [review]
Patches gdata/gdata-upload-stream.c
Comment 7 George Stephanos 2010-11-25 11:40:38 UTC
Comment on attachment 175234 [details] [review]
Patches gdata/gdata-upload-stream.c

I commented out the mutex locks in close_cancelled_cb() in order to place my locks in wrote_chunk_cb (both happen at the same time).
Also, I added an error check between the mutex locks in wrote_chuck_cb() that returns the call (and ceases output) if the error is not NULL.
Comment 8 George Stephanos 2010-11-25 11:54:42 UTC
Created attachment 175235 [details] [review]
Correct patch. Submitted wrong one earlier.

Sorry, I submitted a wrong patch earlier.
Comment 9 Philip Withnall 2010-11-25 14:27:46 UTC
Your report is clear and makes sense, thank you.
Comment 10 Philip Withnall 2010-11-25 14:32:45 UTC
Review of attachment 175235 [details] [review]:

The patch is functionally correct, but there are a few stylistic things which I've commented on. (Don't worry, these don't affect GCI. :-))

If you fancy coming up with a second iteration of the patch, please do, but don't feel you have to. I can make the changes myself sometime later on today. Thanks for working on this!

::: gdata/gdata-upload-stream.c
@@ +474,2 @@
 	/* Set the error and signal that the close operation has finished */
+	//g_static_mutex_lock (&(priv->response_mutex));

Rather than commenting these out, it would've been better to remove them. These changes should've been in a separate patch from the one which modifies wrote_chunk_cb(), since they fix a separate bug.

One advantage of providing patches in git format-patch format is that the patch file includes the commit message, so you can easily provide a detailed explanation of why the changes in the patch were necessary. When dealing with tricky code like this, such explanations are always useful.

@@ +553,3 @@
+		g_static_mutex_lock (&(priv->response_mutex));
+		g_return_if_fail(priv->response_error == NULL);
+		g_static_mutex_unlock (&(priv->response_mutex));

g_return_if_fail() should only be used to detect programmer errors (such as passing NULL as a function argument when the function doesn't accept NULL). Runtime errors should be checked using normal if statements.

I would've moved the check for (priv->response_error == NULL) into the if statement below, e.g.:

if (priv->entry != NULL && priv->response_error == NULL)

then put the g_static_mutex_lock() before the if block, and the g_static_mutex_unlock() after the if block.
Comment 11 Philip Withnall 2010-11-27 23:09:20 UTC
commit 8fb1a2873e39e2e1dae6e405cef756e2a289653b
Author: George Stephanos <gaf.stephanos@gmail.com>
Date:   Sat Nov 27 23:04:16 2010 +0000

    Bug 607620 — Cancelled uploads appear partially complete in PicasaWeb
    
    Ensure that the bottom multipart/related boundary string isn't sent to the
    server for cancelled uploads (or indeed, any uploads which have experienced
    an error). This prevents partial uploads from being interpreted by the server
    as images. Patch based directly on work by George Stephanos
    <gaf.stephanos@gmail.com> for GCI 2010. Closes: bgo#607620

 gdata/gdata-upload-stream.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

commit f80a20656ec3cdabc9882670769ebd331495d2f6
Author: Philip Withnall <philip@tecnocode.co.uk>
Date:   Sat Nov 27 23:00:15 2010 +0000

    core: Prevent deadlock when closing a cancelled GDataUploadStream
    
    When closing a GDataUploadStream which has already been cancelled, the
    cancellation callback would be executed in the same thread as
    gdata_upload_stream_close(), causing a deadlock (as they both tried to hold
    the same lock). The only solution I can think of is to move the cancellation
    out to the main thread, which ensures that deadlock will only happen if the
    application is doing synchronous I/O (i.e. gdata_upload_stream_close() gets
    called in the main thread). If they're doing that, they deserve the deadlock.
    Spotted by George Stephanos <gaf.stephanos@gmail.com>. Helps: bgo#607620

 gdata/gdata-upload-stream.c |   34 ++++++++++++++++++++++++++++++----
 1 files changed, 30 insertions(+), 4 deletions(-)