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 637036 - Overhaul cancellation support
Overhaul cancellation support
Status: RESOLVED FIXED
Product: libgdata
Classification: Platform
Component: General
git master
Other All
: Normal major
: 0.8
Assigned To: libgdata-maint
libgdata-maint
Depends on:
Blocks:
 
 
Reported: 2010-12-11 18:12 UTC by Philip Withnall
Modified: 2010-12-20 12:14 UTC
See Also:
GNOME target: ---
GNOME version: ---



Description Philip Withnall 2010-12-11 18:12:11 UTC
While we've had cancellation support supposedly in the API for a long time, in many cases it doesn't work, even in the core of libgdata.

It needs to be fixed up, and its behaviour made consistent: at the moment, we return a G_IO_ERROR_CANCELLED even if the network communication and post-processing for an operation were successful (e.g. if the operation was cancelled very close to finishing).

It would be better if we returned G_IO_ERROR_CANCELLED if operations are cancelled at any time up to the end of network communication, and returned successfully if they were cancelled any later.

It would also be worth investigating implementing some kind of “poisoning” of network traffic upon cancellation by sending a load of junk at the end of a cancelled request, so that the servers are forced to abandon processing of such requests. This means they won't affect server state unbeknown to the client (because we've returned G_IO_ERROR_CANCELLED).
Comment 1 Philip Withnall 2010-12-11 22:48:59 UTC
(See also: bug #633364.)
Comment 2 Philip Withnall 2010-12-20 12:14:36 UTC
I've just pushed a smorgasbord of commits relating to this. Here's a selection of them:

commit 484f237da97941f486b5fd4fac84f7baa6810f21
Author: Philip Withnall <philip@tecnocode.co.uk>
Date:   Sat Dec 18 17:24:44 2010 +0000

    core: Cancel gdata_download_stream_read() if the entire download is cancelled
    
    Helps: bgo#637036

 gdata/gdata-download-stream.c |   36 ++++++++++++++++++++++++++++++++++--
 1 files changed, 34 insertions(+), 2 deletions(-)

commit 8dca3d4cb09de2787d490f0b62246d2bd22360a7
Author: Philip Withnall <philip@tecnocode.co.uk>
Date:   Thu Dec 16 19:10:47 2010 +0000

    core: Allow gdata_download_stream_close() to be cancelled
    
    Helps: bgo#637036

 gdata/gdata-download-stream.c |  102 ++++++++++++++++++++++++++++++++++++++--
 1 files changed, 96 insertions(+), 6 deletions(-)

commit 4d2061ee2f614f5ff923d2f49f9218a8db38ddfd
Author: Philip Withnall <philip@tecnocode.co.uk>
Date:   Sat Dec 18 12:46:07 2010 +0000

    media: Add a cancellable parameter to gdata_media_thumbnail_download()
    
    Add a cancellable parameter to gdata_media_thumbnail_download() to mirror
    GDataDownloadStream:cancellable. The tests have been updated accordingly.
    
    This breaks the following API:
    • gdata_media_thumbnail_download()
    
    Helps: bgo#637036

 gdata/media/gdata-media-thumbnail.c              |   10 ++++++++--
 gdata/media/gdata-media-thumbnail.h              |    2 +-
 gdata/services/picasaweb/gdata-picasaweb-album.c |    2 +-
 gdata/tests/picasaweb.c                          |    4 ++--
 4 files changed, 12 insertions(+), 6 deletions(-)

commit 63e1cc0b4e8440f851549552da334fe0c58bdc47
Author: Philip Withnall <philip@tecnocode.co.uk>
Date:   Sat Dec 18 12:43:07 2010 +0000

    media: Add a cancellable parameter to gdata_media_content_download()
    
    Add a cancellable parameter to gdata_media_content_download() to mirror
    GDataDownloadStream:cancellable. The tests have been updated accordingly.
    
    This breaks the following API:
    • gdata_media_content_download()
    
    Helps: bgo#637036

 gdata/media/gdata-media-content.c               |   10 ++++++++--
 gdata/media/gdata-media-content.h               |    2 +-
 gdata/services/picasaweb/gdata-picasaweb-file.c |    2 +-
 gdata/tests/picasaweb.c                         |    2 +-
 4 files changed, 11 insertions(+), 5 deletions(-)

commit 032a5bc08986f66735846ffec3d1daafcd62cd0f
Author: Philip Withnall <philip@tecnocode.co.uk>
Date:   Sat Dec 18 12:39:43 2010 +0000

    documents: Add a cancellable parameter to gdata_documents_document_download()
    
    Add a cancellable parameter to gdata_documents_document_download() to mirror
    GDataDownloadStream:cancellable. The tests have been updated accordingly.
    
    This breaks the following API:
    • gdata_documents_document_download()
    
    Helps: bgo#637036

 .../services/documents/gdata-documents-document.c  |   11 +++++++++--
 .../services/documents/gdata-documents-document.h  |    2 +-
 gdata/tests/documents.c                            |    6 +++---
 3 files changed, 13 insertions(+), 6 deletions(-)

commit 002d4d83e03f7bb30544d7690c83e4c44f5ae73f
Author: Philip Withnall <philip@tecnocode.co.uk>
Date:   Sat Dec 18 12:35:54 2010 +0000

    core: Add a “cancellable” property to GDataDownloadStream
    
    Allow the code which creates the GDataDownloadStream to specify a GCancellable
    which will cancel the entire download operation (i.e. permanently cancel all
    I/O on the stream and all network activity).
    
    This breaks the following API:
    • gdata_download_stream_new()
    
    The following API has been added:
    • GDataDownloadStream:cancellable
    • gdata_download_stream_get_cancellable()
    
    Helps: bgo#637036

 docs/reference/gdata-sections.txt                  |    1 +
 gdata/gdata-download-stream.c                      |   83 ++++++++++++++++----
 gdata/gdata-download-stream.h                      |    4 +-
 gdata/gdata.symbols                                |    1 +
 gdata/media/gdata-media-content.c                  |    2 +-
 gdata/media/gdata-media-thumbnail.c                |    2 +-
 .../services/documents/gdata-documents-document.c  |    2 +-
 7 files changed, 76 insertions(+), 19 deletions(-)

commit e74adbd5a414cf075df5fdd868a3cfb628e7f1ac
Author: Philip Withnall <philip@tecnocode.co.uk>
Date:   Fri Dec 17 21:49:06 2010 +0000

    core: Fix cancellation of gdata_upload_stream_close()
    
    It shouldn't return a cancellation error for the entire stream — if
    gdata_upload_stream_close() is cancelled, it merely means to return from the
    call without blocking on the stream actually being closed.
    
    Helps: bgo#637036

 gdata/gdata-upload-stream.c |   68 ++++++++++++++++++++++++++++++++-----------
 1 files changed, 51 insertions(+), 17 deletions(-)

commit b7974ac2ed3293968cfb0c10c610a0afb0d2abc8
Author: Philip Withnall <philip@tecnocode.co.uk>
Date:   Fri Dec 17 23:30:28 2010 +0000

    youtube: Add a cancellable parameter to the upload video method
    
    Add a cancellable parameter to gdata_youtube_service_upload_video() to
    mirror GDataUploadStream:cancellable. The tests have been updated accordingly.
    
    This breaks the following API:
     • gdata_youtube_service_upload_video()
    
    Helps: bgo#637036

 gdata/services/youtube/gdata-youtube-service.c |   11 +++++++++--
 gdata/services/youtube/gdata-youtube-service.h |    3 ++-
 gdata/tests/youtube.c                          |    5 +++--
 3 files changed, 14 insertions(+), 5 deletions(-)

commit c512dd50e0c5c5561dae0ad333cf35e29ff5c1ae
Author: Philip Withnall <philip@tecnocode.co.uk>
Date:   Fri Dec 17 23:28:30 2010 +0000

    picasaweb: Add a cancellable parameter to the upload file method
    
    Add a cancellable parameter to gdata_picasaweb_service_upload_file() to mirror
    GDataUploadStream:cancellable. The tests have been updated accordingly.
    
    This breaks the following API:
     • gdata_picasaweb_service_upload_file()
    
    Helps: bgo#637036

 gdata/services/picasaweb/gdata-picasaweb-service.c |   12 +++++++++---
 gdata/services/picasaweb/gdata-picasaweb-service.h |    2 +-
 gdata/tests/picasaweb.c                            |   12 ++++++------
 3 files changed, 16 insertions(+), 10 deletions(-)

commit 2641a723031b13b129cbe9b8e864115f0024a398
Author: Philip Withnall <philip@tecnocode.co.uk>
Date:   Fri Dec 17 23:26:53 2010 +0000

    documents: Add a cancellable parameter to the upload/update methods
    
    Add a cancellable parameter to gdata_documents_service_upload_document()
    and gdata_documents_service_update_document() to mirror
    GDataUploadStream:cancellable. The tests have been updated accordingly.
    
    This breaks the following API:
     • gdata_documents_service_upload_document()
     • gdata_documents_service_update_document()
    
    Helps: bgo#637036

 gdata/services/documents/gdata-documents-service.c |   24 +++++++++++++++-----
 gdata/services/documents/gdata-documents-service.h |    5 ++-
 gdata/tests/documents.c                            |   14 +++++-----
 3 files changed, 28 insertions(+), 15 deletions(-)

commit 2338e007e768770627158f5298cc44bd6de80ed8
Author: Philip Withnall <philip@tecnocode.co.uk>
Date:   Fri Dec 17 23:13:53 2010 +0000

    core: Add a “cancellable” property to GDataUploadStream
    
    Allow the code which creates the GDataUploadStream to specify a GCancellable
    which will cancel the entire upload operation (i.e. permanently cancel all
    I/O on the stream and all network activity).
    
    This breaks the following API:
     • gdata_upload_stream_new()
    
    The following API has been added:
     • GDataUploadStream:cancellable
     • gdata_upload_stream_get_cancellable()
    
    Helps: bgo#637036

 docs/reference/gdata-sections.txt                  |    1 +
 gdata/gdata-upload-stream.c                        |   79 ++++++++++++++++----
 gdata/gdata-upload-stream.h                        |    3 +-
 gdata/gdata.symbols                                |    1 +
 gdata/services/documents/gdata-documents-service.c |    3 +-
 gdata/services/picasaweb/gdata-picasaweb-service.c |    2 +-
 gdata/services/youtube/gdata-youtube-service.c     |    2 +-
 7 files changed, 72 insertions(+), 19 deletions(-)

commit 89fbed575e1e22c3e68b9bbc1cc4cb93b9800e21
Author: Philip Withnall <philip@tecnocode.co.uk>
Date:   Fri Dec 17 23:11:19 2010 +0000

    core: Cancel gdata_upload_stream_write() if the entire upload is cancelled
    
    Helps: bgo#637036

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

commit 4ca3dbe2b46fab8d578ca9d16ab06fe60be08c26
Author: Philip Withnall <philip@tecnocode.co.uk>
Date:   Fri Dec 17 22:22:24 2010 +0000

    core: Port GDataUploadStream to use the safe message cancellation methods
    
    GDataUploadStream now uses _gdata_service_actually_send_message(), and
    consequently message cancellation is properly an non-racily supported by
    holding a GCancellable in the GDataUploadStream's private data.
    
    Helps: bgo#637036

 gdata/gdata-upload-stream.c |   31 +++++++++++++++++++++++++------
 1 files changed, 25 insertions(+), 6 deletions(-)

commit 8962b93e81f3095cd3c78f0bf2af81c2360311cf
Author: Philip Withnall <philip@tecnocode.co.uk>
Date:   Thu Dec 16 23:37:54 2010 +0000

    core: Fix synchronisation between threads in GDataUploadStream
    
    This upgrades GDataUploadStream to keep track of the number of bytes in
    various buffers, so that it can return more accurate numbers of bytes written
    in each gdata_upload_stream_write() operation, as well as support cancellation
    of a write without cancelling the entire upload.
    
    Helps: bgo#637036

 gdata/gdata-upload-stream.c |   70 ++++++++++++++++++++++++++++++++----------
 1 files changed, 53 insertions(+), 17 deletions(-)

commit 9170ed2d603397a12be1cbfafd6408b7c37de98a
Author: Philip Withnall <philip@tecnocode.co.uk>
Date:   Thu Dec 16 23:37:09 2010 +0000

    core: Don't reach EOF in gdata_buffer_pop_data() unless it's the last read
    
    Helps: bgo#637036

 gdata/gdata-buffer.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

commit 97b1e0dccee1a70a53bd601721219ac1639ed450
Author: Philip Withnall <philip@tecnocode.co.uk>
Date:   Thu Dec 16 22:31:12 2010 +0000

    core: Rearrange the network thread callbacks in GDataUploadStream
    
    If we use wrote-body-data, we have access to the number of bytes which were
    written from the last chunk, which is important for keeping track of the total
    number of bytes written, coming up in the next commit.
    
    Helps: bgo#637036

 gdata/gdata-upload-stream.c |   51 +++++++++++++++++++++++++++++++-----------
 1 files changed, 37 insertions(+), 14 deletions(-)

commit 61f4f916bfb249c8ed6ff2dcfb5c71a47a6ee902
Author: Philip Withnall <philip@tecnocode.co.uk>
Date:   Thu Dec 16 22:06:12 2010 +0000

    core: Don't set the response error when cancelling gdata_upload_stream_write()
    
    Helps: bgo#637036

 gdata/gdata-upload-stream.c |   50 +++++++++++++++++++++++-------------------
 1 files changed, 27 insertions(+), 23 deletions(-)

commit 76ddf92f954288ff187b69328e37317829f24ce9
Author: Philip Withnall <philip@tecnocode.co.uk>
Date:   Thu Dec 16 19:22:09 2010 +0000

    core: Prevent a race condition in joining GDataUploadStream's network thread
    
    Helps: bgo#637036

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

commit 1d32533173641103283c2fc201821cd50e7f180d
Author: Philip Withnall <philip@tecnocode.co.uk>
Date:   Thu Dec 16 19:16:58 2010 +0000

    core: Add some comments about the lifecycle of GDataDownloadStream
    
    Helps: bgo#637036

 gdata/gdata-download-stream.c |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)

commit 7dfcadbc0e34e378eff8890a269e91cc0dab298e
Author: Philip Withnall <philip@tecnocode.co.uk>
Date:   Thu Dec 16 18:27:34 2010 +0000

    core: Port GDataDownloadStream to use the safe message cancellation methods
    
    GDataDownloadStream now uses _gdata_service_actually_send_message(), and
    consequently message cancellation is properly an non-racily supported by
    holding a GCancellable in the GDataDownloadStream's private data.
    
    Helps: bgo#637036

 gdata/gdata-download-stream.c |   51 +++++++++++++++++++++++++++++------------
 1 files changed, 36 insertions(+), 15 deletions(-)

commit ffde18eaeeb03464ad183cc46c41ed10d03f57a9
Author: Philip Withnall <philip@tecnocode.co.uk>
Date:   Thu Dec 16 18:05:25 2010 +0000

    core: Expose _gdata_service_actually_send_message() internally
    
    This will allow the GDataUploadStream and GDataDownloadStream message-sending
    code to be rebased on it to fix cancellation races.
    
    Helps: bgo#637036

 gdata/gdata-private.h |    1 +
 gdata/gdata-service.c |   16 ++++++++--------
 2 files changed, 9 insertions(+), 8 deletions(-)

commit fbf02481e91a4bc0aea673dc1a971063426ec3fd
Author: Philip Withnall <philip@tecnocode.co.uk>
Date:   Thu Dec 16 17:59:12 2010 +0000

    core: Prevent cancellation races being caused by other messages being queued
    
    The SoupSession::request-queued signal is emitted for messages other than
    the one we're preventing message cancellation races for, so we need to check
    that it's been emitted for the message we're interested in.
    
    Helps: bgo#637036

 gdata/gdata-service.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

commit 0edc7d221cf99bd701d4912e4ba1546797978544
Author: Philip Withnall <philip@tecnocode.co.uk>
Date:   Thu Dec 16 17:50:06 2010 +0000

    core: Handle GDataDownloadStreams which never have a network thread started
    
    Helps: bgo#637036

 gdata/gdata-download-stream.c |   22 ++++++++++++++--------
 1 files changed, 14 insertions(+), 8 deletions(-)

commit 3e9e1dbfa7b8a2853635aa3f0ea64421290490b5
Author: Philip Withnall <philip@tecnocode.co.uk>
Date:   Thu Dec 16 17:36:04 2010 +0000

    core: Block gdata_download_stream_close() on completion of network activity
    
    It wasn't previously waiting for the network activity to finish, which it
    should (if any has started).
    
    Helps: bgo#637036

 gdata/gdata-download-stream.c |   33 +++++++++++++++++++++++++++++----
 1 files changed, 29 insertions(+), 4 deletions(-)

commit 458b9cc57e2a9a734a591a3fe4e0955b3decd401
Author: Philip Withnall <philip@tecnocode.co.uk>
Date:   Thu Dec 16 16:41:44 2010 +0000

    core: Remove redundant cancellation checks from the authentication thread
    
    Helps: bgo#637036

 gdata/gdata-service.c |   11 ++---------
 1 files changed, 2 insertions(+), 9 deletions(-)

commit 7111756f40c188479364830b1da819e3e8cf39f9
Author: Philip Withnall <philip@tecnocode.co.uk>
Date:   Thu Dec 16 16:31:11 2010 +0000

    core: Fix cancellation handling for closing a GDataUploadStream
    
    Deadlocks could have occurred if the cancellable was in the process of being
    cancelled when g_cancellable_disconnect() was called (as it was being called
    with the same mutex locked that the cancellation callback attempts to lock).
    
    Additionally, the suboptimal method of running the cancellation code in an
    idle callback to ensure that it wasn't run in the same thread as
    gdata_upload_stream_close() can be removed, because the cancellation handler
    is now installed before gdata_upload_stream_close() locks the response_mutex,
    thus removing the possibility of deadlock if the cancellable has already been
    cancelled.
    
    Helps: bgo#637036

 gdata/gdata-upload-stream.c |   69 ++++++++++++++++++-------------------------
 1 files changed, 29 insertions(+), 40 deletions(-)

commit 80be38cfb29922a4b0846c1b5496ba3cd3f1f5f4
Author: Philip Withnall <philip@tecnocode.co.uk>
Date:   Thu Dec 16 16:07:10 2010 +0000

    core: Fix cancellation handling for writing to a GDataUploadStream
    
    There was a minor locking issue with write_finished, and deadlocks could have
    occurred if the cancellable was in the process of being cancelled when
    g_cancellable_disconnect() was called (as it was being called with the same
    mutex locked that the cancellation callback attempts to lock).
    
    Helps: bgo#637036

 gdata/gdata-upload-stream.c |   15 +++++++++------
 1 files changed, 9 insertions(+), 6 deletions(-)

commit 321abcef328f35b2cd732fea7e3e2ccfd96ccf4d
Author: Philip Withnall <philip@tecnocode.co.uk>
Date:   Thu Dec 16 15:45:50 2010 +0000

    core: Fix error handling for reading from a GDataDownloadStream
    
    Cancellation should only return an error if it couldn't read a single byte.
    All errors should be accompanied by a return value of -1, not 0.

 gdata/gdata-download-stream.c |   17 ++++++++++++-----
 1 files changed, 12 insertions(+), 5 deletions(-)

commit 76e7ba27e5f275fdd886648b00e0d7ea0e2147b4
Author: Philip Withnall <philip@tecnocode.co.uk>
Date:   Thu Dec 16 15:25:45 2010 +0000

    core: Fix liveness issues and cancellation problems in GDataBuffer
    
    There was a race condition with cancellation of a gdata_buffer_pop_data()
    operation where if the operation was cancelled before the first g_cond_wait()
    call, the thread would sit in the g_cond_wait() for ever, as the GCond would
    never be signalled in the cancellation handler, as it had already run.
    
    This commit fixes the problem by serialising access to the cancelled flag,
    ensuring that deadlock doesn't occur by setting up and tearing down the
    cancellation stuff without the buffer's mutex being held.
    
    Helps: bgo#637036

 gdata/gdata-buffer.c |   49 +++++++++++++++++++++++++++----------------------
 1 files changed, 27 insertions(+), 22 deletions(-)