GNOME Bugzilla – Bug 764077
Cancellation doesn't work
Last modified: 2016-06-16 13:05:24 UTC
In grilo-test-ui, load a source with a lot of entries, such as grl-guardianvideos-lua and press the back button while it's still loading: (grilo-test-ui-0.3:30905): Grilo-CRITICAL **: grl_operation_get_private_data: assertion 'data != NULL' failed (grilo-test-ui-0.3:30905): Grilo-WARNING **: [source] grl-source.c:2317: Source 'grl-guardianvideos-lua' emitted 'remaining=0' more than once for operation 6 (grilo-test-ui-0.3:30905): Grilo-CRITICAL **: grl_operation_get_private_data: assertion 'data != NULL' failed (grilo-test-ui-0.3:30905): Grilo-WARNING **: [source] grl-source.c:2317: Source 'grl-guardianvideos-lua' emitted 'remaining=0' more than once for operation 6 (grilo-test-ui-0.3:30905): Grilo-CRITICAL **: grl_operation_get_private_data: assertion 'data != NULL' failed (grilo-test-ui-0.3:30905): Grilo-WARNING **: [source] grl-source.c:2317: Source 'grl-guardianvideos-lua' emitted 'remaining=0' more than once for operation 6 etc.
Created attachment 324633 [details] [review] lua-factory: support to cancel operation Grilo has optional method to cancel operations which Lua-Factory had not implemented so far, till now. For each operation we create a GCancellabe that should be used in async functions in Lua-Library such as grl.fetch and grl.unzip. The grl_lua_operations_cancel_operation(), will cancel the operation, remove the OperationSpec from its internals and free the related data.
Review of attachment 324633 [details] [review]: That's not how you check for cancellation. The correct way is: if (!grl_net_wc_request_finish (GRL_NET_WC (source_object), res, &data, &len, &err)) { if (!g_error_matches (err, G_IO_ERROR, G_IO_ERROR_CANCELLED)) { g_warning ("Error that is not cancellation: %s", err->message); } return; } ::: src/lua-factory/grl-lua-library.c @@ +478,3 @@ gchar *fixed = NULL; + if (g_cancellable_is_cancelled (fo->cancellable)) { No.
(In reply to Bastien Nocera from comment #2) > Review of attachment 324633 [details] [review] [review]: > > That's not how you check for cancellation. > > The correct way is: > if (!grl_net_wc_request_finish (GRL_NET_WC (source_object), > res, &data, &len, &err)) { > if (!g_error_matches (err, G_IO_ERROR, G_IO_ERROR_CANCELLED)) { > g_warning ("Error that is not cancellation: %s", err->message); > } > return; > } > > ::: src/lua-factory/grl-lua-library.c > @@ +478,3 @@ > gchar *fixed = NULL; > > + if (g_cancellable_is_cancelled (fo->cancellable)) { > > No. :-) Thanks, I'll fix it!
Created attachment 324973 [details] [review] net: Use proper error when operation is cancelled
Created attachment 324974 [details] [review] lua-factory: check return value for error handling On grl_net_wc_request_finish function we should check for return value instead of trusting directly the Error value.
Created attachment 324975 [details] [review] lua-factory: support to cancel operation Grilo has optional method to cancel operations which Lua-Factory had not implemented so far, till now. For each operation we create a GCancellabe that should be used in async functions in Lua-Library such as grl.fetch and grl.unzip. The grl_lua_operations_cancel_operation(), will cancel the operation, remove the OperationSpec from its internals and free the related data.
Hi, (In reply to Bastien Nocera from comment #2) > Review of attachment 324633 [details] [review] [review]: > > That's not how you check for cancellation. > > The correct way is: > if (!grl_net_wc_request_finish (GRL_NET_WC (source_object), > res, &data, &len, &err)) { > if (!g_error_matches (err, G_IO_ERROR, G_IO_ERROR_CANCELLED)) { > g_warning ("Error that is not cancellation: %s", err->message); > } > return; > } I was wondering why is it necessary to check the error code/domain in order to verify that an operation was cancelled, could you please clarify? In case of GrlNetWc, it has its own error code/domain and it was not setting it correctly (first patch). I still needed the g_cancellable_is_cancelled for one situation: It is fine to return some data if we are fetching multiple urls and some them failed but it is not okay to return any data if the operation was cancelled.
Review of attachment 324973 [details] [review]: ::: libs/net/grl-net-wc.c @@ +639,3 @@ + if (error->code == G_IO_ERROR_CANCELLED) { + g_simple_async_result_set_error (result, GRL_NET_WC_ERROR, + GRL_NET_WC_ERROR_CANCELLED, Could we not use G_IO_ERROR/G_IO_ERROR_CANCELLED here? We'd need another patch to say that we won't use GRL_NET_WC_ERROR_CANCELLED, but the gio error instead.
Review of attachment 324974 [details] [review]: ::: src/lua-factory/grl-lua-library.c @@ +641,3 @@ guint len, i; + if (err != NULL) { + GRL_WARNING ("Can't fetch zip file (URL: %s): '%s'", uo->url, err->message); Here you'd need a: if (!g_error_matches (err, G_IO_ERROR, G_IO_ERROR_CANCELLED)) @@ +642,3 @@ + if (err != NULL) { + GRL_WARNING ("Can't fetch zip file (URL: %s): '%s'", uo->url, err->message); + g_error_free (err); You need to return and avoid calling lua, don't you?
Review of attachment 324975 [details] [review]: ::: src/lua-factory/grl-lua-library-operations.c @@ +47,3 @@ g_clear_pointer (&os->string, g_free); g_clear_object (&os->options); + g_clear_object (&os->cancellable); Also make sure to cancel it before unref'ing it, just in case. So: if (os->cancellable) { g_cancellable_cancel (); g_object_unref (); } ::: src/lua-factory/grl-lua-library.c @@ +480,3 @@ if (!grl_net_wc_request_finish (GRL_NET_WC (source_object), res, &data, &len, &err)) { + if (g_error_matches (err, GRL_NET_WC_ERROR, GRL_NET_WC_ERROR_CANCELLED)) { Let's use the G_IO_ERROR one instead. @@ +502,3 @@ + * successfully fetched prior a task cancellation, we should double check + * here and not call source's callback function */ + if (g_cancellable_is_cancelled (fo->cancellable)) No, you'd have got a cancellation error earlier if it was actually cancelled.
(In reply to Bastien Nocera from comment #10) > Review of attachment 324975 [details] [review] [review]: > > ::: src/lua-factory/grl-lua-library-operations.c > @@ +47,3 @@ > g_clear_pointer (&os->string, g_free); > g_clear_object (&os->options); > + g_clear_object (&os->cancellable); > > Also make sure to cancel it before unref'ing it, just in case. > > So: > if (os->cancellable) { > g_cancellable_cancel (); > g_object_unref (); > } Sure! > > ::: src/lua-factory/grl-lua-library.c > @@ +480,3 @@ > if (!grl_net_wc_request_finish (GRL_NET_WC (source_object), > res, &data, &len, &err)) { > + if (g_error_matches (err, GRL_NET_WC_ERROR, > GRL_NET_WC_ERROR_CANCELLED)) { > > Let's use the G_IO_ERROR one instead. Agreed! > > @@ +502,3 @@ > + * successfully fetched prior a task cancellation, we should double check > + * here and not call source's callback function */ > + if (g_cancellable_is_cancelled (fo->cancellable)) > > No, you'd have got a cancellation error earlier if it was actually cancelled. You are right. But I recall to test this and have issues. I'll blame GRL_NET_WC_ERROR_CANCELLED for that :) I've tested again a few times and seems fine. -- (In reply to Bastien Nocera from comment #9) > Review of attachment 324974 [details] [review] [review]: > > ::: src/lua-factory/grl-lua-library.c > @@ +641,3 @@ > guint len, i; > + if (err != NULL) { > + GRL_WARNING ("Can't fetch zip file (URL: %s): '%s'", uo->url, > err->message); > > Here you'd need a: > if (!g_error_matches (err, G_IO_ERROR, G_IO_ERROR_CANCELLED)) > > @@ +642,3 @@ > + if (err != NULL) { > + GRL_WARNING ("Can't fetch zip file (URL: %s): '%s'", uo->url, > err->message); > + g_error_free (err); > > You need to return and avoid calling lua, don't you? Yes. I squashed both patches now!
Created attachment 328530 [details] [review] lua-factory: support to cancel operation Grilo has optional method to cancel operations which Lua-Factory had not implemented so far, till now. For each operation we create a GCancellabe that should be used in async functions in Lua-Library such as grl.fetch and grl.unzip. The grl_lua_operations_cancel_operation(), will cancel the operation, remove the OperationSpec from its internals and free the related data.
Created attachment 328531 [details] [review] net: Set proper error when operation is cancelled
Created attachment 328532 [details] [review] net: use G_IO_ERROR on cancelled operation By deprecating GRL_NET_WC_ERROR_CANCELLED we can use G_IO_ERROR_CANCELLED instead.
Review of attachment 324973 [details] [review]: obsolete.
Review of attachment 328530 [details] [review]: Looks good.
Review of attachment 328531 [details] [review]: ::: libs/net/grl-net-wc.c @@ +638,2 @@ if (error) { + if (error->code == G_IO_ERROR_CANCELLED) { you need to check the domain as well, as, numerically, G_IO_ERROR_CANCELLED could correspond to something else. So g_error_matches (error, G_IO_ERROR, G_IO_ERROR_CANCELLED)... @@ +639,3 @@ + if (error->code == G_IO_ERROR_CANCELLED) { + g_simple_async_result_set_error (result, G_IO_ERROR, + G_IO_ERROR_CANCELLED, In which case you could propagate the error.
Review of attachment 328532 [details] [review]: Looks good. Any chance to replace the other errors with G_IO_ERROR_ ones?
(In reply to Bastien Nocera from comment #18) > Review of attachment 328532 [details] [review] [review]: > > Looks good. Any chance to replace the other errors with G_IO_ERROR_ ones? Yes, I think a few of them could be replaced. I'll take a look on it
Created attachment 329136 [details] [review] net: use G_IO_ERROR on cancelled operation By deprecating GRL_NET_WC_ERROR_CANCELLED we can use G_IO_ERROR_CANCELLED instead. --diff in version: using g_error_match and g_simple_async_result_set_from_error
Created attachment 329137 [details] [review] net: Set proper error when operation is cancelled --diff in version: using g_error_match and g_simple_async_result_set_from_error
Created attachment 329138 [details] [review] net: use G_IO_ERROR_PROXY_FAILED instead of GRL_NET_WC_ERROR_PROXY_ERROR
Created attachment 329139 [details] [review] net: use G_IO_ERROR_FAILED instead of GRL_NET_WC_ERROR_UNAVAILABLE
Review of attachment 329138 [details] [review]: Sure.
Review of attachment 329139 [details] [review]: ::: libs/net/grl-net-wc.c @@ +690,3 @@ g_simple_async_result_set_error (G_SIMPLE_ASYNC_RESULT (result), + G_IO_ERROR, + G_IO_ERROR_FAILED, G_IO_ERROR_INVALID_ARGUMENT instead?
Review of attachment 329136 [details] [review]: ::: libs/net/grl-net-wc.c @@ +592,3 @@ if (error) { + if (g_error_matches (error, G_IO_ERROR, G_IO_ERROR_CANCELLED)) { + g_simple_async_result_set_from_error (result, error); You're changing the error message here.
Review of attachment 329137 [details] [review]: Yep.
Attachment 329137 [details] pushed as 3c6f1de - net: Set proper error when operation is cancelled Attachment 329138 [details] pushed as 92dadb2 - net: use G_IO_ERROR_PROXY_FAILED instead of GRL_NET_WC_ERROR_PROXY_ERROR
Comment on attachment 328530 [details] [review] lua-factory: support to cancel operation Attachment 328530 [details] pushed as 173958f - lua-factory: support to cancel operation
Created attachment 329174 [details] [review] net: use G_IO_ERROR on cancelled operation By deprecating GRL_NET_WC_ERROR_CANCELLED we can use G_IO_ERROR_CANCELLED instead.
Created attachment 329175 [details] [review] net: use G_IO_ERROR_FAILED instead of GRL_NET_WC_ERROR_UNAVAILABLE
Review of attachment 329174 [details] [review]: Yep.
Review of attachment 329175 [details] [review]: Sure.
Attachment 329174 [details] pushed as 1a89d35 - net: use G_IO_ERROR on cancelled operation Attachment 329175 [details] pushed as ea1f423 - net: use G_IO_ERROR_FAILED instead of GRL_NET_WC_ERROR_UNAVAILABLE