Created attachment 234123 [details] [review]
Patch

Comment on attachment 234123 [details] [review]
Patch

>+	/* The SoupMessage could have been cancelled in between, so we
>+	 * must ensure that we do not end up calling
>+	 * async_send_request_return_result() twice for the same request.
>+	 */
>+	if (g_task_had_error (task))
>+		return FALSE;

Hm... I don't think that's right. If you just cancel the cancellable passed to soup_session_send_async() then g_task_had_error() would be TRUE, but the callback won't get run.

I think the problem here is that soup_session_async_cancel_message() in soup-session-async.c shouldn't be forcibly finishing the message if it's in the SOUP_MESSAGE_CACHED state. Or at least, it should be doing something different.

Also, the SOUP_CACHE_RESPONSE_NEEDS_VALIDATION codepath looks like it has basically the same problem... if the parent message is cancelled, then we need to cancel (or at least abandon) the validation request too.

(In reply to comment #2)
> (From update of attachment 234123 [details] [review])
> >+	/* The SoupMessage could have been cancelled in between, so we
> >+	 * must ensure that we do not end up calling
> >+	 * async_send_request_return_result() twice for the same request.
> >+	 */
> >+	if (g_task_had_error (task))
> >+		return FALSE;
> 
> Hm... I don't think that's right. If you just cancel the cancellable passed to
> soup_session_send_async() then g_task_had_error() would be TRUE, but the
> callback won't get run.

Well in my tests the callback was run and causing crashes so I think the SoupMessage was directly cancelled.

> I think the problem here is that soup_session_async_cancel_message() in
> soup-session-async.c shouldn't be forcibly finishing the message if it's in the
> SOUP_MESSAGE_CACHED state. Or at least, it should be doing something different.
> 
> Also, the SOUP_CACHE_RESPONSE_NEEDS_VALIDATION codepath looks like it has
> basically the same problem... if the parent message is cancelled, then we need
> to cancel (or at least abandon) the validation request too.

Right. So in any case I think that the cancellation must take precedence over any cache stuff going on (as it has to act transparently). In the case of RESPONSE_FRESH I guess it's enough to save the source id and cancel it before it happens, and for RESPONSE_NEEDS_VALIDATION I guess we have to cancel the revalidation request as you said.

(In reply to comment #3)
> > Hm... I don't think that's right. If you just cancel the cancellable passed to
> > soup_session_send_async() then g_task_had_error() would be TRUE, but the
> > callback won't get run.
> 
> Well in my tests the callback was run and causing crashes so I think the
> SoupMessage was directly cancelled.

Right, I meant, your patch works if the caller calls soup_session_cancel_message(), but it doesn't work if they cancel the cancellable instead; in that case the queue item just gets stranded.

I'll managed to code a test for these errors. Will attach it here along with a proposed patch for the bug.

Created attachment 235012 [details] [review]
Make SoupSession's GTask always use SoupMessageQueueItem's cancellable

If the client does not provide a cancellable there is no way to cancel the SoupSession internal GTask. We need it to perform cancellations with soup_session_cancel_message().

Created attachment 235016 [details] [review]
Properly handle cancellations for resources returned by the SoupCache

Cancel should work now for resources that are either fresh (returned immediately) or that need revalidation (via conditional requests).

Created attachment 235019 [details] [review]
Add cancellation support to soup_test_request_send()

Needed to cancel requests in tests.

Created attachment 235020 [details] [review]
New cancellation test for cache-test.c

Tests, among other stuff, the originally reported bug.

Comment on attachment 235012 [details] [review]
Make SoupSession's GTask always use SoupMessageQueueItem's cancellable

seems right

Comment on attachment 235016 [details] [review]
Properly handle cancellations for resources returned by the SoupCache

>+} AsyncRespondData;

Sounds more generic than it really is... AsyncCacheCancelData?

>+		soup_cache_cancel_conditional_request (data->cache, data->conditional_msg);
>+
>+	/* The original request message should be marked as cancelled. */

So, conditional_get_ready_cb() is still going to run after this. The way it's written now, I guess everything will work out, but it's fragile. It would be better to *just* do soup_cache_cancel_condition_request() here, and then have conditional_get_cb() deal with both the successful and cancelled cases.

For the idle_return_from_cache case... actually you don't even need to connect to the cancellable; the timeout source is going to run in the very next iteration of the loop anyway, so just check g_cancellable_is_cancelled() at the top of idle_return_from_cache_cb(), and then do the right thing from there.

>+	soup_session_process_queue_item (data->item->session, data->item, NULL, FALSE);

(if we were going to keep cancelled_cb() the way it is, this should be kick_queue() instead, so it would still DTRT even if you cancelled from another thread.)

>+static void
>+free_async_respond_data (AsyncRespondData *data)

Style nitpick: put this before cancelled_cb() (ie, immediately after the struct declaration, and before anything that uses the struct).

>+	g_signal_handlers_disconnect_matched (item->cancellable, G_SIGNAL_MATCH_FUNC, 0, 0,
>+					      NULL, cancelled_cb, NULL);

Use g_cancellable_disconnect() for race-safetiness.

>+	stream = soup_cache_send_response (cache, item->msg);
>+	if (!stream) {
>+		/* FIXME: the cache should remove the entry and
>+		 * correctly process the request.

If you leave the soup_cache_send_response() call in async_respond_from_cache() (and store the stream as the task_data) then you won't have to worry about this.

>+		if (!conditional_msg) {
>+			free_async_respond_data (data);
> 			return FALSE;

especially if you end up not using the async data in the fresh case, it would be good to reorganize the code to create it after this point, so you don't end up just creating and then freeing it.


Special bonus comment:

> 		soup_session_queue_message (session, conditional_msg,
> 					    conditional_get_ready_cb,
> 					    item);

While rereading all this code, I realized that we can rewrite this to use soup_session_send_async() instead, and then if you get back a 200 response instead of a 304, you'll have a stream you can return to the caller, fixing the FIXME in conditional_get_ready_cb(). (But that's not part of this bug, just something we should do later.)

Comment on attachment 235019 [details] [review]
Add cancellation support to soup_test_request_send()

>+++ b/tests/test-utils.c
>@@ -7,6 +7,8 @@
> #include <locale.h>
> #include <signal.h>
> 
>+#include "libsoup/soup-request-http.h"

Not needed. test-utils.h includes everything

>+	union {
>+		SoupRequest  *req;
>+		GCancellable *cancellable;
>+	} cancellable;

It's just a test program. We don't have to worry about conserving memory. No need for a union.

>+typedef enum {
>+  NO_CANCEL,
>+  MSG_CANCEL,
>+  CANCELLABLE_CANCEL
>+} TestCancellation;

I would make this:

  typedef enum {
    SOUP_TEST_REQUEST_NONE = 0,
    SOUP_TEST_REQUEST_CANCEL_MESSAGE = (1 << 0),
    SOUP_TEST_REQUEST_CANCEL_CANCELLABLE = (1 << 1)
  } SoupTestRequestFlags;

But just pass "0", not SOUP_TEST_REQUEST_NONE in the places that aren't doing cancellation.

Comment on attachment 235019 [details] [review]
Add cancellation support to soup_test_request_send()

Hm... actually:

>-		g_timeout_add (100, cancel_request_timeout, cancellable);

>+		g_timeout_add_full (G_PRIORITY_HIGH, 0, cancel_request_timeout, cancel_data, NULL);

The original misc-test test is supposed to be testing the case where you cancel while reading the response body (which had a bug in it at one point), but changing this from 100 to 0 means we're now testing something different.

Having separate CANCEL_IMMEDIATE and CANCEL_SOON (or something) flags would be one way to solve this. (And actually, then you could also make do_cancel_while_reading_test() test both IMMEDIATE and SOON.)

Comment on attachment 235020 [details] [review]
New cancellation test for cache-test.c

Yay tests.

I just skimmed this, but it looks good (subject to any API modifications made to the previous patch)

(In reply to comment #11)
> (From update of attachment 235016 [details] [review])
> >+	stream = soup_cache_send_response (cache, item->msg);
> >+	if (!stream) {
> >+		/* FIXME: the cache should remove the entry and
> >+		 * correctly process the request.
> 
> If you leave the soup_cache_send_response() call in async_respond_from_cache()
> (and store the stream as the task_data) then you won't have to worry about
> this.
> 
> >+		if (!conditional_msg) {
> >+			free_async_respond_data (data);
> > 			return FALSE;
> 
> especially if you end up not using the async data in the fresh case, it would
> be good to reorganize the code to create it after this point, so you don't end
> up just creating and then freeing it.

One of the reasons to remove that code from async_respond_from_cache() was to avoid setting task data twice, because soup_session_send_finish() is going to be called either if the request succeeds or if it's cancelled, so the task data could be either a SoupMessageQueueItem or the SendAsyncCacheData.

ah, well, you've got to do something about the FIXME...

I guess you could just do

  g_object_set_data_full (G_OBJECT (task), "stream", stream, g_object_unref);

or something rather than g_task_set_task_data().

Created attachment 235142 [details] [review]
Properly handle cancellations for resources returned by the SoupCache

Created attachment 235143 [details] [review]
Add cancellation support to soup_test_request_send()

(In reply to comment #16)
> ah, well, you've got to do something about the FIXME...
> 
> I guess you could just do
> 
>   g_object_set_data_full (G_OBJECT (task), "stream", stream, g_object_unref);
> 
> or something rather than g_task_set_task_data().

Yeah, that's what I did in the end.

Comment on attachment 235142 [details] [review]
Properly handle cancellations for resources returned by the SoupCache

ok, looks good