GNOME Bugzilla – Bug 637036
Overhaul cancellation support
Last modified: 2010-12-20 12:14:36 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).
(See also: bug #633364.)
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(-)