GNOME Bugzilla – Bug 763908
Cancel or remove pending internal asynchronous operations or sources during destruction
Last modified: 2018-01-23 09:59:13 UTC
We cancel (g_cancellable_cancel) or remove (g_source_remove) pending internal asynchronous operations or sources when an object is destroyed. Right now, we hold a reference to the object itself (g_object_ref (self)) to delay the destruction while something is pending. This approach is easier to implement, but has a few drawbacks: (a) Code outside such objects (ie. their clients) are not expecting them to stay alive after they have dropped all their references. Therefore, if the pending asynchronous operation or source has any visible side-effects (eg., emitting a signal) then bad things can happen (eg., crashes). (b) All objects might not be destroyed during shutdown. This is usually not a big issue because the process is about to die anyway. In our case, though, gegl_exit complains if some GeglBuffers were still alive. (c) Not cancelling or removing pending items is a bit wasteful. I have a few patches to get this started. Feel free to pick it up. Just make sure that you split it into smaller patches (eg., one for object or operation/source) so that it is easier to review and test against regressions.
Created attachment 324323 [details] [review] collection-icon-watcher: Rename a variable
Created attachment 324324 [details] [review] collection-icon-watcher: Cancel cursor_next_async during destruction
Created attachment 324325 [details] [review] embed: Don't leak self when load_show_id is removed before the timeout
Created attachment 324326 [details] [review] embed: Remove load_show_id during destruction
Created attachment 324327 [details] [review] preview-nav-buttons: Remove auto_hide_id during destruction
Created attachment 324438 [details] [review] tracker-controller: Rename a variable
Created attachment 324439 [details] [review] tracker-controller: Cancel cursor_next_async during destruction
Created attachment 324440 [details] [review] export-notification: Remove timeout_id during destruction
Created attachment 324441 [details] [review] indexing-notification: Remove timeout_id during destruction
Created attachment 324442 [details] [review] delete-notification: Remove timeout_id during destruction
Created attachment 324443 [details] [review] done-notification: Remove timeout_id during destruction
Created attachment 324446 [details] [review] offset-controller: Rename variable
Created attachment 324447 [details] [review] offset-controller: Check for error from sparql_cursor_next_finish
Created attachment 324448 [details] [review] offset-controller: Cancel cursor_next_async during destruction
Created attachment 324449 [details] [review] offset-controller: Cancel cursor_next_async during destruction attaching correct version of the patch.
Created attachment 324452 [details] [review] offset-controller: Cancel tracker_queue_select during destruction
Created attachment 324474 [details] [review] done-notification: Cancel item_process_async during destruction
Created attachment 324475 [details] [review] done-notification: Remove duplicated g_application_release
Created attachment 324476 [details] [review] done-notification: Cancel pipeline_save_async during destruction
Created attachment 324477 [details] [review] tool-crop: Cancel item_process_async during destruction
Created attachment 324479 [details] [review] properties-dialog: Move common code to its own function
Created attachment 324480 [details] [review] properties-dialog: Remove title_entry_timeout during destruction
Created attachment 324481 [details] [review] tracker-change-monitor: Move code to its own function
Created attachment 324482 [details] [review] tracker-change-monitor: Remove pending_events_id during destruction
Created attachment 324485 [details] [review] preview-view: Cancel pipeline_save_async during destruction
Review of attachment 324446 [details] [review]: ++
Review of attachment 324438 [details] [review]: ++ By convention, we don't add append a full stop (ie. '.') to the subject of the commit message. See: https://wiki.gnome.org/Git/CommitMessages
Created attachment 324488 [details] [review] tracker-controller: Rename cancellable
Created attachment 324489 [details] [review] tracker-controller: Cancel cursor_next_async during destruction Attaching correct version of the patch.
Created attachment 324490 [details] [review] tracker-controller: Clear queue_cancellable on destruction
Review of attachment 324447 [details] [review]: Thanks for the patch, Rafael. Looks good except one small issue: ::: src/photos-offset-controller.c @@ +75,3 @@ + if (error != NULL) + { + g_warning ("Unable to query item: %s", error->message); We are trying to query the number of items in a particular mode (overview, favorites, collections, search), not an individual item. Therefore, something like "Unable to query the item count" would be more correct.
Review of attachment 324449 [details] [review]: ::: src/photos-offset-controller.c @@ +78,3 @@ + { + g_error_free (error); + return; Pedantically speaking, we should also use 'goto out' here for the tracker_sparql_cursor_close call. Doing that will simplify the error handling a bit. The bigger question is, whether we need to explicitly close the cursor at all? My understanding of the TrackerSparqlCursor implementation is that it will close the cursor when it is destroyed due to the last reference being dropped. We can address that in a separate patch, though. @@ +118,3 @@ + tracker_sparql_cursor_next_async (cursor, + priv->cancellable, self->priv->cancellable, else it won't compile. :) @@ +150,3 @@ priv = self->priv; + priv->cancellable = g_cancellable_new (); 'cancellable' is missing from the Private struct. :)
Created attachment 324528 [details] [review] offset-controller: Handle errors from tracker_sparql_cursor_next_finish Fixed and pushed.
Created attachment 324529 [details] [review] offset-controller: Cancel cursor_next_async during destruction Fixed and pushed.
Review of attachment 324452 [details] [review]: Thanks for the patch, Rafael. This patch looks OK, but I think it has exposed a bug in tracker_sparql_connection_query_async/finish. Namely, that it doesn't throw G_IO_ERROR_CANCELLED if the operation is cancelled. This means that we try to use an invalid PhotosOffsetController, if the application is quit while the call was still pending. A quick look at the generated C code for the corresponding Vala bits didn't reveal any use of g_simple_async_result_set_check_cancellable, which makes me suspicious. (The Vala compiler still generates code using GSimpleAsyncResult, not GTask.) Let's sort this out before pushing this.
Created attachment 324532 [details] [review] preview-view: Cancel photos_base_item_process_async during destruction
Created attachment 324536 [details] [review] offset-controller: Cancel cursor_next_async during destruction Addressing raised points.
Review of attachment 324536 [details] [review]: I had already pushed a fixed version of this patch. Sorry for the confusion.
Created attachment 324544 [details] [review] local-item: Cancel trash_item during destruction
Review of attachment 324544 [details] [review]: Thanks for the patch. A few minor nits, but otherwise looks perfect. ::: src/photos-local-item.c @@ +177,3 @@ + const gchar *uri; + + self = PHOTOS_LOCAL_ITEM (user_data); Nitpick: since we are only assigning self inside the if, would be better to move the definition here too. @@ +207,2 @@ static void +photos_local_item_dispose (GObject *object) Nitpick: would be nice to move it lower to retain the alphabetical ordering.
Created attachment 324583 [details] [review] local-item: Cancel g_file_trash_async during destruction Fixed and pushed.
Review of attachment 324477 [details] [review]: ::: src/photos-tool-crop.c @@ +907,3 @@ + g_error_free (error); + return; + } This can be simplified, as in some of the other patches.
Created attachment 324586 [details] [review] tool-crop: Cancel item_process_async during destruction Fixed and pushed.
Review of attachment 324485 [details] [review]: ::: src/photos-preview-view.c @@ +528,1 @@ g_application_hold (app); I would also remove the g_application_hold/release. It made sense when we used to hold our own reference to PreviewView. If someone quit the application (which means MainWindow getting destroyed), then this would keep the process alive until the callback was invoked, and the ref on PreviewView would ensure that the callback didn't run into memory errors, even though some of the widgets outside PreviewView might already be gone. Now, with the ref on PreviewView gone, it doesn't make sense to keep the process alive. In future, we need a more elaborate structure in place to stop the main window from disappearing during such operations. We can take ideas from gedit, for example.
Created attachment 324621 [details] [review] base-item: Cancel g_file_query_info_async during destruction
Review of attachment 324621 [details] [review]: The patch looks fine, except that it doesn't compile. See below. ::: src/photos-base-item.c @@ +679,1 @@ info = g_file_query_info_finish (file, res, &error); We were leaking info. :( @@ +683,3 @@ + { + self = PHOTOS_BASE_ITEM (user_data); + priv = self->priv; I find it cleaner to assign self and priv in only place in the function. See some of the other async operations for ideas. @@ +684,3 @@ + self = PHOTOS_BASE_ITEM (user_data); + priv = self->priv; + g_warning ("Unable to query info for file at %s: %s", priv->uri, error->message); eg., we can use g_file_get_uri instead of priv->uri, since priv won't be defined here. @@ +1507,3 @@ + { + g_cancellable_cancel (self->cancellable); + g_clear_object (&self->cancellable); s/self/priv/g
Created attachment 324669 [details] [review] base-item: Don't leak the GFileInfo
Created attachment 324671 [details] [review] base-item: Cancel g_file_query_info_async during destruction Fixed and pushed.
Review of attachment 324479 [details] [review]: Thanks, Rafael. Looks good except one minor issue. ::: src/photos-properties-dialog.c @@ -116,3 @@ photos_utils_set_edited_name (self->urn, new_title); - g_object_unref (self); This should be part of the next patch.
Created attachment 324735 [details] [review] Cancel g_file_read_async during destruction
Created attachment 324736 [details] [review] base-item: Cancel g_file_read_async during destruction
Created attachment 324738 [details] [review] base-item: Cancel g_file_read_async during destruction
Created attachment 324739 [details] [review] base-item: Check for errors while loading the item for printing
Created attachment 324741 [details] [review] Don't leak GFileInfo
Created attachment 324742 [details] [review] base-item: Cancel photos_base_item_refresh_icon during destruction
Created attachment 324746 [details] [review] base-item: Cancel gdk_pixbuf_new_from_stream_async during destruction
Review of attachment 324746 [details] [review]: ::: src/photos-base-item.c @@ +607,3 @@ + priv = self->priv; + + file = G_FILE (g_object_get_data (G_OBJECT (stream), "file")); Why are you separating the error condition handling in two parts? This block should be joined with the one above. It's all about the same error handling: failed to get a thumbnail, delete the file, set the fail condition, leave. That's how it was before. You just need to add the early leave in case the object is being destroyed.
Review of attachment 324741 [details] [review]: r+
Review of attachment 324739 [details] [review]: ::: src/photos-base-item.c @@ +1483,3 @@ GtkPrintOperationResult print_res; + node = photos_base_item_load_finish (self, res, &error); Note that error is being used below for the gtk_print_operation_run. I think it's a good idea to set it to NULL before that, just to make sure we're not re-using the value somehow.
Review of attachment 324479 [details] [review]: ::: src/photos-properties-dialog.c @@ +108,2 @@ +static void +photos_properties_dialog_remove_timout (PhotosPropertiesDialog *self) Typo alert: "remove_timeout", not "remove_timout". :)
Created attachment 325149 [details] [review] properties-dialog: Move common code to its own function Fixed and pushed.
Created attachment 325150 [details] [review] properties-dialog: Remove title_entry_timeout during destruction Rebased on top of master.
Review of attachment 325150 [details] [review]: Thanks, Rafael. There is one problem: ::: src/photos-properties-dialog.c @@ +521,3 @@ } + photos_properties_dialog_remove_timeout (self); Unlike most timeouts, photos_properties_dialog_title_entry_timeout has a side-effect. ie. it calls photos_utils_set_edited_name to change the title/name of the item in tracker's database. So, if the dialog was getting destroyed before the timeout had a chance to run, we would be missing the update. Therefore, I think, we should somehow do the update in dispose. We need to be careful, though, because dispose can get called multiple times. eg., with a if (title_entry_timeout != 0).
(In reply to Debarshi Ray from comment #63) > Review of attachment 325150 [details] [review] [review]: > > Thanks, Rafael. There is one problem: > > ::: src/photos-properties-dialog.c > @@ +521,3 @@ > } > > + photos_properties_dialog_remove_timeout (self); > > Unlike most timeouts, photos_properties_dialog_title_entry_timeout has a > side-effect. ie. it calls photos_utils_set_edited_name to change the > title/name of the item in tracker's database. So, if the dialog was getting > destroyed before the timeout had a chance to run, we would be missing the > update. > > Therefore, I think, we should somehow do the update in dispose. We need to > be careful, though, because dispose can get called multiple times. eg., with > a if (title_entry_timeout != 0). So if I want to discard the modification I have to manually revert the name to what it was before?
Review of attachment 324442 [details] [review]: Looks good to me.
Review of attachment 324440 [details] [review]: Looks good to me.
(In reply to Rafael Fonseca from comment #64) > (In reply to Debarshi Ray from comment #63) > > Review of attachment 325150 [details] [review] [review] [review]: > > ::: src/photos-properties-dialog.c > > @@ +521,3 @@ > > } > > > > + photos_properties_dialog_remove_timeout (self); > > > > Unlike most timeouts, photos_properties_dialog_title_entry_timeout has a > > side-effect. ie. it calls photos_utils_set_edited_name to change the > > title/name of the item in tracker's database. So, if the dialog was getting > > destroyed before the timeout had a chance to run, we would be missing the > > update. > > > > Therefore, I think, we should somehow do the update in dispose. We need to > > be careful, though, because dispose can get called multiple times. eg., with > > a if (title_entry_timeout != 0). > > So if I want to discard the modification I have to manually revert the name > to what it was before? The entry in the properties dialog is instant apply. ie. it updates the title as you type. The timeout is there to batch the keystrokes. So, currently, there is no way to undo the modification. Maybe we can present an in-app notification with an "undo" button once you close the dialog? We can decide it later as it is not relevant to this bug. :) I was worried about the case where the user types and manages to close the dialog before the timeout. It is hard to physically achieve that feat, but who knows? In that case, we will miss the last few keystrokes if we don't do the update during destruction. eg., the title could be left as "new ti" instead of "new title".
Review of attachment 324443 [details] [review]: ::: src/photos-done-notification.c @@ +209,3 @@ + self->timeout_id = g_timeout_add_seconds (DONE_TIMEOUT, + photos_done_notification_timeout, + self); Could be on the same line. Given all the long GObject/C names and modern terminals being able to accommodate more than 80 characters, it is OK to go till 120 characters in the code.
Created attachment 325162 [details] [review] done-notification: Remove timeout_id during destruction Fixed the formatting and pushed.
Review of attachment 324441 [details] [review]: Looks good to me.
Created attachment 325192 [details] [review] Fix the reference counting for NotificationManager
Review of attachment 324746 [details] [review]: For some reason this is not applying anymore. :/ ::: src/photos-base-item.c @@ +607,3 @@ + priv = self->priv; + + if (pixbuf == NULL) The problem is that the non-cancelled error path needs self. So we either need to duplicate the self/priv assignment, or we have to split the error path.
Created attachment 325405 [details] [review] base-item: Cancel gdk_pixbuf_new_from_stream_async during destruction Rebased and pushed.
Review of attachment 324482 [details] [review]: Thanks Rafael. Looks good to me.
Review of attachment 324481 [details] [review]: ++
Created attachment 325418 [details] [review] properties-dialog: remove title_entry_timeout during destruction New patch fixing raised issue.
Created attachment 325429 [details] [review] properties-dialog: Remove title_entry_timeout during destruction Fixed version.
Review of attachment 325429 [details] [review]: Looks good to me. By the way, this also fixes leaking a reference to PropertiesDialog when requeuing the timeout. :)
Review of attachment 324738 [details] [review]: Thanks, Umang. A few comments: ::: src/photos-base-item.c @@ +621,1 @@ PhotosBaseItemPrivate *priv = self->priv; We can't touch self and priv before we have handled any possible errors. If we received a G_IO_ERROR_CANCELLED, then they are already destroyed. Surely you know about commit 5eead6b0772b0d4d8b2ac60ae41b615956a7697a :) @@ +643,2 @@ } This is where we should assign self and priv. @@ +644,3 @@ + if (stream == NULL) + { Broken alignment. @@ +647,3 @@ + priv->failed_thumbnailing = TRUE; + priv->thumb_path = NULL; + g_file_delete_async (file, G_PRIORITY_DEFAULT, NULL, NULL, NULL); Nitpick: put the g_file_delete_async in the error handling block above like in commit 5eead6b0772b0d4d8b2ac60ae41b615956a7697a @@ +659,2 @@ out: + g_object_unref (stream); This isn't quite right. If you put it after 'out', which is a good idea, then you need to use g_clear_object, because stream can be NULL and g_object_unref doesn't like NULL inputs. Also, this bit is an unrelated change, so it's better to use a separate patch.
Created attachment 325769 [details] [review] base-item: Shuffle some code around This is the g_object_unref to g_clear_object part of the previous patch.
Review of attachment 324739 [details] [review]: ::: src/photos-base-item.c @@ +1493,2 @@ if (node == NULL) goto out; If you are already checking 'error', then this is redundant.
Created attachment 325771 [details] [review] base-item: Check for errors while loading the item for printing
Created attachment 325772 [details] [review] base-item: Reset the GError before using it Addressing Rafael's comment about the resetting the GError, which I missed earlier.
Review of attachment 324742 [details] [review]: Thanks, looks good to me. Pushed.
Created attachment 325814 [details] [review] base-item: Cancel g_file_read_async during destruction
Review of attachment 325814 [details] [review]: Thanks, Umang. Unfortunately, this version introduces two new problems: ::: src/photos-base-item.c @@ +655,3 @@ + g_free (uri); + g_error_free (error); + goto out; This goto can't be here because we want to keep going. @@ +693,2 @@ photos_base_item_refresh_thumb_path_read, + self); The corresponding g_object_unref (self) needs to be removed from the callback.
Created attachment 325819 [details] [review] base-item: Cancel g_file_read_async during destruction
Review of attachment 325819 [details] [review]: Looks good to me.
Review of attachment 324474 [details] [review]: ::: src/photos-done-notification.c @@ +163,1 @@ g_application_hold (app); Same thing as comment 44
Review of attachment 324476 [details] [review]: ::: src/photos-done-notification.c @@ +139,1 @@ g_application_hold (app); Same thing as comment 44
Review of attachment 324475 [details] [review]: Apart from the following comment, this depends on the other two patches touching this code. ::: src/photos-done-notification.c @@ -107,3 @@ - app = g_application_get_default (); - g_application_release (app); We have 2 holds (one for processing and one for saving), so we need 2 releases, right? Or am I missing something?
Created attachment 326094 [details] [review] preview-view: Cancel pipeline_save_async during destruction Addressing raised points.
Created attachment 326096 [details] [review] done-notification: cancel item_process_async during destruction Fixing issues.
Created attachment 326097 [details] [review] done-notification: Cancel pipeline_save_async during destruction Fixing issues.
Created attachment 329969 [details] [review] preview-view: Cancel pipeline_save_async during destruction I took the liberty to tweak this patch to leverage the new API in bug 764076. I added a separate warning to indicate a cancellation due to unintended destruction of PhotosPreviewView in the middle of the critical pipeline_save_async operation. If this works out, then we should adjust the rest of the patches involving similar critical operations (and close this bug).
Created attachment 329988 [details] [review] preview-view: Hold the UI while saving the pipeline
Comment on attachment 329988 [details] [review] preview-view: Hold the UI while saving the pipeline Pushed to master.
Review of attachment 326096 [details] [review]: There is a problem with this. I am sorry for not noticing this earlier. ::: src/photos-done-notification.c @@ +155,2 @@ photos_done_notification_edit_undo_process, + self); The base_item_process_async call will always be immediately cancelled because ... @@ +158,1 @@ photos_done_notification_destroy (self); ... this will call gtk_widget_destroy, which in turn will call the dispose vfunc and cancel the GCancellable. One option is to gtk_widget_hide the notification here, and only call photos_done_notification_destroy once everything is done. If we do that then we must be careful to call it during the error paths too. Another option, probably a better one, would be to turn this into an action. Let's say app.edit-undo that takes the URN/ID of the BaseItem as a string parameter. Then one of the global-ish classes (eg., PhotosApplication) can handle it. That will decouple the life time of the notification from that of the asynchronous operation.
Review of attachment 326097 [details] [review]: Turning this whole operation into a GAction might be a better option. See attachment 326096 [details] [review]
Review of attachment 324489 [details] [review]: ::: src/photos-tracker-controller.c @@ +172,3 @@ photos_item_manager_add_item (PHOTOS_ITEM_MANAGER (priv->item_mngr), cursor); tracker_sparql_cursor_next_async (cursor, + self->priv->cursor_cancellable, This does subtly change the logic, doesn't it? Maybe we need a chained GCancellable that can (a) be directly cancelled, and (b) be cancelled as a result of cancelling another GCancellable. Few months ago I was asking on IRC for some prior art regarding this, but even though pbor said that this is a valid approach, I couldn't find any code samples. Shouldn't be too hard to implement it as a GCancellable sub-class, though. @@ +197,2 @@ tracker_sparql_cursor_next_async (cursor, + self->priv->cursor_cancellable, Same here.
Created attachment 331470 [details] [review] application, done-notification: Use a GAction to handle the revert
Created attachment 331900 [details] [review] done-notification: Remove unnecessary references
Created attachment 331901 [details] [review] application, done-notification: Use a GAction to handle the revert
Created attachment 341124 [details] [review] tracker-controller: Don't use g_cancellable_reset
Created attachment 341125 [details] [review] tracker-controller: Handle tracker_sparql_cursor_next_finish errors
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/gnome-photos/issues/43.