Merge lp:~rbalint/update-manager/fix-stale-pkg-refs into lp:update-manager

[Balint Reczey]

[Balint Reczey (rbalint)]

I'm also checking rdeps if they need update to adapt to this change.

[Balint Reczey (rbalint)]

I considered replacing pkg with a proxy object, but the code would have been more confusing imo.

[Brian Murray (brian-murray)]

review: Approve

[Julian Andres Klode (juliank)]

So, does this work reliably if a package disappears between stages? I'm not sure it can even haooen.

[Balint Reczey (rbalint)]

It crashes with exception when a package is going away like before but this is very rare and is not a regression. We can fix that later.

[Balint Reczey (rbalint)]

Julian provided and alternative fix that updates referenced pkg objects upon cache.open() which is fairly fast and would fix the issue in a different way.

Unmerged revisions

2824. By Balint Reczey on 2018-05-31

Update changelog

2823. By Balint Reczey on 2018-05-31

Store package name and cache reference in UpdateItem to avoid invalid package references

LP: #1773316

=== modified file 'UpdateManager/Core/UpdateList.py'
--- UpdateManager/Core/UpdateList.py	2018-03-21 17:41:10 +0000
+++ UpdateManager/Core/UpdateList.py	2018-05-31 12:03:14 +0000
@@ -43,29 +43,42 @@
 
 
 class UpdateItem():
-    def __init__(self, pkg, name, icon, to_remove):
+    def __init__(self, pkg, cache, name, icon, to_remove):
         self.icon = icon
         self.name = name
-        self.pkg = pkg
+        self.cache = cache
+        if pkg is not None:
+            self.pkg_name = pkg.name
+        else:
+            self.pkg_name = None
         self.to_remove = to_remove
 
     def is_selected(self):
         if not self.to_remove:
-            return self.pkg.marked_install or self.pkg.marked_upgrade
+            return (self.get_pkg().marked_install or
+                    self.get_pkg().marked_upgrade)
         else:
-            return self.pkg.marked_delete
+            return self.get_pkg().marked_delete
+
+    def get_pkg(self):
+        if self.cache is None:
+            return None
+        try:
+            return self.cache[self.pkg_name]
+        except KeyError:
+            return None
 
 
 class UpdateGroup(UpdateItem):
     _depcache = {}
 
-    def __init__(self, pkg, name, icon, to_remove):
-        UpdateItem.__init__(self, pkg, name, icon, to_remove)
+    def __init__(self, pkg, cache, name, icon, to_remove):
+        UpdateItem.__init__(self, pkg, cache, name, icon, to_remove)
         self._items = set()
         self._deps = set()
         self.core_item = None
         if pkg is not None:
-            self.core_item = UpdateItem(pkg, name, icon, to_remove)
+            self.core_item = UpdateItem(pkg, cache, name, icon, to_remove)
             self._items.add(self.core_item)
 
     @property
@@ -77,7 +90,7 @@
     def add(self, pkg, cache=None, eventloop_callback=None, to_remove=False):
         name = utils.get_package_label(pkg)
         icon = Gio.ThemedIcon.new("package")
-        self._items.add(UpdateItem(pkg, name, icon, to_remove))
+        self._items.add(UpdateItem(pkg, cache, name, icon, to_remove))
         # If the pkg is in self._deps, stop here. We have already calculated
         # the recursive dependencies for this package, no need to do it again.
         if cache and pkg.name in cache and pkg.name not in self._deps:
@@ -93,8 +106,8 @@
 
     def _init_deps(self, cache, eventloop_callback):
         for item in self._items:
-            if item.pkg and item.pkg.name not in self._deps:
-                self._add_deps(item.pkg, cache, eventloop_callback)
+            if item.pkg_name is not None and item.pkg_name not in self._deps:
+                self._add_deps(item.get_pkg(), cache, eventloop_callback)
 
     def _add_deps(self, pkg, cache, eventloop_callback):
         """Adds pkg and dependencies of pkg to the dependency list."""
@@ -148,23 +161,24 @@
             return 0
         size = 0
         for item in self.items:
-            size += getattr(item.pkg.candidate, "size", 0)
+            size += getattr(item.get_pkg().candidate, "size", 0)
         return size
 
 
 class UpdateApplicationGroup(UpdateGroup):
-    def __init__(self, pkg, application, to_remove):
+    def __init__(self, pkg, cache, application, to_remove):
         name = application.get_display_name()
         icon = application.get_icon()
-        super(UpdateApplicationGroup, self).__init__(pkg, name, icon,
+        super(UpdateApplicationGroup, self).__init__(pkg, cache, name, icon,
                                                      to_remove)
 
 
 class UpdatePackageGroup(UpdateGroup):
-    def __init__(self, pkg, to_remove):
+    def __init__(self, pkg, cache, to_remove):
         name = utils.get_package_label(pkg)
         icon = Gio.ThemedIcon.new("package")
-        super(UpdatePackageGroup, self).__init__(pkg, name, icon, to_remove)
+        super(UpdatePackageGroup, self).__init__(pkg, cache, name, icon,
+                                                 to_remove)
 
 
 class UpdateSystemGroup(UpdateGroup):
@@ -173,7 +187,8 @@
         # the core components and packages.
         name = _("%s base") % utils.get_ubuntu_flavor_name(cache=cache)
         icon = Gio.ThemedIcon.new("distributor-logo")
-        super(UpdateSystemGroup, self).__init__(None, name, icon, to_remove)
+        super(UpdateSystemGroup, self).__init__(None, None, name, icon,
+                                                to_remove)
 
 
 class UpdateOrigin():
@@ -429,7 +444,7 @@
         for pkg in pkgs:
             app = self._get_application_for_package(pkg)
             if app is not None:
-                app_group = UpdateApplicationGroup(pkg, app, to_remove)
+                app_group = UpdateApplicationGroup(pkg, cache, app, to_remove)
                 app_groups.append(app_group)
             else:
                 ungrouped_pkgs.append(pkg)
@@ -450,20 +465,21 @@
         if ungrouped_pkgs:
             # Separate out system base packages. If we have already found an
             # application for all updates, don't bother.
-            meta_group = UpdateGroup(None, None, None, to_remove)
+            meta_group = UpdateGroup(None, None, None, None, to_remove)
             flavor_package = utils.get_ubuntu_flavor_package(cache=cache)
             meta_pkgs = [flavor_package, "ubuntu-standard", "ubuntu-minimal"]
             meta_pkgs.extend(self._get_linux_packages())
             for pkg in meta_pkgs:
                 if pkg in cache:
-                    meta_group.add(cache[pkg])
+                    meta_group.add(cache[pkg], cache)
             for pkg in ungrouped_pkgs:
                 if meta_group.is_dependency(pkg, cache, eventloop_callback):
                     if system_group is None:
                         system_group = UpdateSystemGroup(cache, to_remove)
-                    system_group.add(pkg)
+                    system_group.add(pkg, cache)
                 else:
-                    pkg_groups.append(UpdatePackageGroup(pkg, to_remove))
+                    pkg_groups.append(UpdatePackageGroup(pkg, cache,
+                                                         to_remove))
 
         app_groups.sort(key=lambda a: a.name.lower())
         pkg_groups.sort(key=lambda a: a.name.lower())
 
=== modified file 'UpdateManager/UpdatesAvailable.py'
--- UpdateManager/UpdatesAvailable.py	2018-03-18 21:43:58 +0000
+++ UpdateManager/UpdatesAvailable.py	2018-05-31 12:03:14 +0000
@@ -407,14 +407,13 @@
         path = model.get_path(iter)
 
         requires_restart = False
-        if data.item and data.item.pkg:
-            requires_restart = self.pkg_requires_restart(data.item.pkg)
+        if data.item:
+            requires_restart = self.pkg_requires_restart(data.item.get_pkg())
         elif data.group:
             if not self.treeview_update.row_expanded(path):
                 # A package in the group requires restart
                 for group_item in data.group.items:
-                    if group_item.pkg and self.pkg_requires_restart(
-                            group_item.pkg):
+                    if self.pkg_requires_restart(group_item.get_pkg()):
                         requires_restart = True
                         break
 
@@ -431,7 +430,7 @@
         activatable = False
         inconsistent = False
         if data.item:
-            activatable = data.item.pkg.name not in self.list.held_back
+            activatable = data.item.pkg_name not in self.list.held_back
             inconsistent = False
         elif data.group:
             activatable = True
@@ -534,16 +533,16 @@
         if (item is None and data.group is not None and
                 data.group.core_item is not None):
             item = data.group.core_item
-        if (item is None or item.pkg is None or
-                item.pkg.candidate is None or
-                item.pkg.candidate.description is None):
+        pkg = None if item is None else item.get_pkg()
+        if pkg is None or pkg.candidate is None or \
+           pkg.candidate.description is None:
             changes_buffer = self.textview_changes.get_buffer()
             changes_buffer.set_text("")
             desc_buffer = self.textview_descr.get_buffer()
             desc_buffer.set_text("")
             self.notebook_details.set_sensitive(False)
             return
-        long_desc = item.pkg.candidate.description
+        long_desc = pkg.candidate.description
         self.notebook_details.set_sensitive(True)
         # do some regular expression magic on the description
         # Add a newline before each bullet
@@ -560,7 +559,7 @@
         desc_buffer.set_text(long_desc)
 
         # now do the changelog
-        name = item.pkg.name
+        name = item.pkg_name
         if name is None:
             return
 
@@ -713,9 +712,9 @@
                 return
             pkg = None
             if data.item:
-                pkg = data.item.pkg
+                pkg = data.item.get_pkg()
             elif data.group and data.group.core_item:
-                pkg = data.group.core_item.pkg
+                pkg = data.group.core_item.get_pkg()
             if pkg and self.pkg_requires_restart(pkg):
                 nonlocal requires_restart
                 requires_restart = True
@@ -953,12 +952,12 @@
         for item in items:
             try:
                 if keep_packages:
-                    item.pkg.mark_keep()
-                elif item.pkg.name not in self.list.held_back:
+                    item.get_pkg().mark_keep()
+                elif item.pkg_name not in self.list.held_back:
                     if not item.to_remove:
-                        item.pkg.mark_install()
+                        item.get_pkg().mark_install()
                     else:
-                        item.pkg.mark_delete()
+                        item.get_pkg().mark_delete()
             except SystemError:
                 pass
 
@@ -1030,7 +1029,8 @@
                 item_row = [
                     item.name,
                     UpdateData(None, None, item),
-                    humanize_size(getattr(item.pkg.candidate, "size", 0)),
+                    humanize_size(getattr(item.get_pkg().candidate,
+                                          "size", 0)),
                     True
                 ]
                 self.store.append(group_iter, item_row)
 
=== modified file 'debian/changelog'
--- debian/changelog	2018-05-28 14:07:41 +0000
+++ debian/changelog	2018-05-31 12:03:14 +0000
@@ -1,3 +1,14 @@
+update-manager (1:18.10.2) UNRELEASED; urgency=medium
+
+  [ Steve Langasek ]
+  * Fix code comment to document the actual proxy behavior, which is correct.
+
+  [ Balint Reczey ]
+  * Store package name and cache reference in UpdateItem to avoid stale
+    package references (LP: #1773316)
+
+ -- Balint Reczey <[e-mail]>  Thu, 31 May 2018 13:59:25 +0200
+
 update-manager (1:18.10.1) cosmic; urgency=medium
 
   * Block style context changed signal while enforcing the main window's
 
=== modified file 'tests/test_update_list.py'
--- tests/test_update_list.py	2017-08-07 19:42:06 +0000
+++ tests/test_update_list.py	2018-05-31 12:03:14 +0000
@@ -59,7 +59,8 @@
         ignored_srcs = set([pkg.candidate.source_name for pkg in
                             self.updates_list.ignored_phased_updates])
         group = self.updates_list.update_groups[0]
-        install_srcs = set([x.pkg.candidate.source_name for x in group.items])
+        install_srcs = set(
+            [x.get_pkg().candidate.source_name for x in group.items])
         self.assertEqual(ignored_srcs, set({'zsh'}))
         self.assertEqual(install_srcs, set({'apt'}))
         self.assertTrue(len(ignored_srcs & install_srcs) == 0)
@@ -142,8 +143,8 @@
         group = self.updates_list.update_groups[0]
         self.assertIsInstance(group, UpdateList.UpdateApplicationGroup)
         self.assertIsNotNone(group.core_item)
-        self.assertEqual(group.core_item.pkg.name, 'installed-app')
-        self.assertListEqual([x.pkg.name for x in group.items],
+        self.assertEqual(group.core_item.pkg_name, 'installed-app')
+        self.assertListEqual([x.pkg_name for x in group.items],
                              ['installed-app'])
 
     def test_app_with_subitems(self):
@@ -151,9 +152,9 @@
         group = self.updates_list.update_groups[1]
         self.assertIsInstance(group, UpdateList.UpdateApplicationGroup)
         self.assertIsNotNone(group.core_item)
-        self.assertEqual(group.core_item.pkg.name,
+        self.assertEqual(group.core_item.pkg_name,
                          'installed-app-with-subitems')
-        self.assertListEqual([x.pkg.name for x in group.items],
+        self.assertListEqual([x.pkg_name for x in group.items],
                              ['installed-app-with-subitems',
                               'installed-pkg-single-dep'])
 
@@ -162,8 +163,8 @@
         group = self.updates_list.update_groups[2]
         self.assertIsInstance(group, UpdateList.UpdatePackageGroup)
         self.assertIsNotNone(group.core_item)
-        self.assertEqual(group.core_item.pkg.name, 'installed-pkg')
-        self.assertListEqual([x.pkg.name for x in group.items],
+        self.assertEqual(group.core_item.pkg_name, 'installed-pkg')
+        self.assertListEqual([x.pkg_name for x in group.items],
                              ['installed-pkg'])
 
     def test_pkg_multiple_deps(self):
@@ -171,9 +172,9 @@
         group = self.updates_list.update_groups[3]
         self.assertIsInstance(group, UpdateList.UpdatePackageGroup)
         self.assertIsNotNone(group.core_item)
-        self.assertEqual(group.core_item.pkg.name,
+        self.assertEqual(group.core_item.pkg_name,
                          'installed-pkg-multiple-deps')
-        self.assertListEqual([x.pkg.name for x in group.items],
+        self.assertListEqual([x.pkg_name for x in group.items],
                              ['installed-pkg-multiple-deps'])
 
     def test_security(self):
@@ -181,7 +182,7 @@
         group = self.updates_list.security_groups[0]
         self.assertIsInstance(group, UpdateList.UpdateSystemGroup)
         self.assertIsNone(group.core_item)
-        self.assertListEqual([x.pkg.name for x in group.items], ['base-pkg'])
+        self.assertListEqual([x.pkg_name for x in group.items], ['base-pkg'])
 
 
 if __name__ == "__main__":