flatpak-dir: Avoid false "changed" notitications for empty installations #5252
Conversation
When an installation is completely empty, with no refs installed and with no remotes configured, its corresponding FlatpakDir is created periodically. That can cause false "changed" notifications for the installation, due to unconditional call to flatpak_dir_mark_changed(), which is meant only to create the file, if it does not exist.
…e_repo() Check multiple times whether the passed in cancellable is not cancelled, to avoid unneeded work and to catch properly whether a file exists or not, because the g_file_query_exists() returns FALSE also when the GFile exists, but the cancellable is cancelled.
| /* Create .changed file early to avoid polling non-existing file in monitor */ | ||
| if (!flatpak_dir_mark_changed (self, &my_error)) | ||
| if (!g_file_test (g_file_peek_path (changed_file), G_FILE_TEST_IS_REGULAR) && |
There was a problem hiding this comment.
Since you have a GFile, you can just use g_file_query_exists()
| if (g_cancellable_set_error_if_cancelled (cancellable, error)) | ||
| return FALSE; |
There was a problem hiding this comment.
If I understand correctly, the sprinkling of the cancellable checks is because this runs in a thread and the cancellable could have been cancelled at any moment. Maybe the first one should explicitly comment this.
There was a problem hiding this comment.
I will say my personal opinion is that each function that takes a GCancellable should have this check at the start of it. For example all of the OStree functions. But I understand the practicality of spreading it out here to make a change in one place.
There was a problem hiding this comment.
Correct. Furthermore, this is only a single place I faced the problem with. To be complete, one might spread the same checking all around the sources, but I did not dare to do it.
This is related to downstream bug:
https://gitlab.gnome.org/GNOME/gnome-software/-/issues/1989
The gnome-software could pass an already cancelled GCancellable into the libflatpak functions, which can end at
_flatpak_dir_ensure_repo, which unconditionally callsflatpak_dir_mark_changed()to create the.changedfile, even when it exists, because the earlier call tog_file_query_exists()returnsFALSEalso when therepodirGFileexists, but thecancellableis cancelled.This exhibits the most with empty installations, which have no refs installed nor any remote configured, because (it seems, though I did not check it in the code) the FlatpakDir is freed early when the installation is completely empty (like my
--userinstallation here). Having there at least a remote configured theFlatpakDiris not freed early, thus it's not recreated on demand.The first commit adds a test to not call
flatpak_dir_mark_changed()when the file already exists. The second commit adds checks for the cancelled state of the cancellable, which can stop the function earlier. As it's not assured when exactly the other thread can cancel the cancellable, I added it into multiple places.One such place, where the FlatpakDir instance is periodically recreated, is when calling
flatpak_installation_list_remotes(), as shown in the following backtrace snippet:The false "changed" notifications are wrong for the gnome-software, because there is no granularity on the signal (having a set of flags for "app/remote $ID added/removed/updated" would be a great improvement), the gnome-software simply reloads everything in the GUI, which can take its time and which is pretty bad user experience, but as there's currently no easy way to recognize what precisely changed, then there's no better option either.