GNOME Bugzilla – Bug 779660
egg-task-cache: Fix cancellation when multiple tasks are queued for a given key
Last modified: 2017-03-07 11:52:35 UTC
Currently, when multiple tasks are queued for a given key, the task that actually fetches the value uses the GCancellable passed by the first caller. The GCancellables used by the other callers are ignored. This has multiple problems: (a) If any caller other than the first is cancelled, it would continue to wait for the value to be fetched before returning. (b) Even if only the first caller is cancelled, but not any other, it will still prevent everybody from getting a value. This isn't ideal. Ideally, whenever a queued caller's GCancellable is cancelled, it should be removed from the queue and returned as soon as possible. However, it shouldn't affect any other task in the queue. If, and only if, all the queued tasks are cancelled, we should cancel fetching the value.
Created attachment 347326 [details] [review] egg-task-cache: Add a missing typecast
Created attachment 347328 [details] [review] egg-task-cache: Track the in-flight GTask used to fetch the value
Created attachment 347332 [details] [review] egg-task-cache: Set up and invoke the in-flight GTask in two steps
Created attachment 347333 [details] [review] egg-task-cache: Fix cancellation when tasks are queued for a given key
Review of attachment 347326 [details] [review]: Seems fine, but are these really required? I can't believe we would do a g_clear_pointer() not as a macro when possible so we get type-checking, argh!
Review of attachment 347328 [details] [review]: LGTM
Review of attachment 347332 [details] [review]: LGTM
Review of attachment 347333 [details] [review]: Seems fine, but if you're not using C++ we should tweak a few things for Builder code base (We don't do casts for void*) ::: contrib/egg/egg-task-cache.c @@ -427,0 +469,10 @@ +egg_task_cache_cancelled_cb (GCancellable *cancellable, + gpointer user_data) +{ ... 7 more ... Are you using this from a C++ compiler by chance? These should be fine without (*) casts.
Attachment 347326 [details] pushed as 9564d61 - egg-task-cache: Add a missing typecast Attachment 347328 [details] pushed as 4ef15fc - egg-task-cache: Track the in-flight GTask used to fetch the value Attachment 347332 [details] pushed as bc6c880 - egg-task-cache: Set up and invoke the in-flight GTask in two steps Attachment 347333 [details] pushed as f8762c9 - egg-task-cache: Fix cancellation when tasks are queued for a given key
Created attachment 347340 [details] [review] egg-task-cache: use g_clear_pointer() Just convenient, has extra race defensiveness, and preferred style for Builder.
Created attachment 347341 [details] [review] egg-task-cache: handle cancelled tasks when freeing cancelled state If we reached this via the cancellation of the task (and then free'ing of the GTask), then we shouldn't try to disconnect or we'll dead-lock.
Created attachment 347342 [details] [review] egg-task-cache: assertions and cast cleanup Be extra careful so we can catch bugs early with assertions.
Created attachment 347343 [details] [review] egg-task-cache: use guint for iter We use < for comparison and GPtrArray.len is uint, so it's perfectly safe to use the same width integer here.
This should fix up a few of those issues and the deadlock I ran into with Builder. Attachment 347340 [details] pushed as e6b93cf - egg-task-cache: use g_clear_pointer() Attachment 347341 [details] pushed as 8bb871e - egg-task-cache: handle cancelled tasks when freeing cancelled state Attachment 347342 [details] pushed as 9666e7b - egg-task-cache: assertions and cast cleanup Attachment 347343 [details] pushed as 75f7bfd - egg-task-cache: use guint for iter
Review of attachment 347341 [details] [review]: ::: contrib/egg/egg-task-cache.c @@ +242,3 @@ + + g_clear_object (&cancelled->cancellable); + } I was, mistakenly, relying on the fact that g_task_return_error_if_cancelled will invoke the GTask's caller and unref it in a separate iteration of the GMainLoop. I forgot that we are past the iteration of the GMainLoop where the GTask was created, so it will complete immediately. However, I am not sure that it is correct to not disconnect from the GCancellable if the GTask was aborted. In the very least, we are leaking the GCancellable, and weird things like g_cancellable_reset might mean that we will be accessing invalid memory if it is cancelled again. Maybe the solution would be to use an idle source in egg_task_cache_cancelled_cb to handle the cancellation in a separate iteration of the GMainLoop?
Review of attachment 347342 [details] [review]: ::: contrib/egg/egg-task-cache.c @@ +488,3 @@ + g_assert (data != NULL); + g_assert (data->self == self); + g_assert (!data->cancellable || G_IS_CANCELLABLE (data->cancellable)); It will be stricter to assert: data->cancellable == cancellable
Created attachment 347371 [details] [review] egg-task-cache: use g_clear_pointer
Created attachment 347372 [details] [review] egg-task-cache: use a stricter assertion for the GCancellable
Created attachment 347374 [details] [review] egg-task-cache: handle the cancellation in an idle callback
Review of attachment 347374 [details] [review]: LGTM
Review of attachment 347372 [details] [review]: LGTM
Review of attachment 347371 [details] [review]: LGTM
Everything looks good, reasoning seems to make sense to me, thanks! Attachment 347371 [details] pushed as 48e09d9 - egg-task-cache: use g_clear_pointer Attachment 347372 [details] pushed as 5ad9526 - egg-task-cache: use a stricter assertion for the GCancellable Attachment 347374 [details] pushed as 32b5234 - egg-task-cache: handle the cancellation in an idle callback
Thanks for another prompt review! \m/