[PATCH] Remove threading from getBaseRepo handling (#1011555)

Brian C. Lane bcl at redhat.com
Fri Nov 8 01:06:06 UTC 2013


From: "Brian C. Lane" <bcl at redhat.com>

The original purpose of the getBaseRepo cache was to ensure that
requests it weren't constantly sitting on the _yum_lock. This was
accomplished with a combination of a cache, decorators to invalidate the
cache and a thread to re-validate the cache.

The problem is that the thread can be fired off at any time, and when
this happens in the middle of a PAYLOAD_MD thread update it can change
the state of yum's preconf/conf attributes, resulting in a traceback
the next time it tries to access .preconf

This patchset hopefully simplifies things a bit by removing the
threading from the picture. Now what happens is:

Any method decorated with @invalidate_br_cache ill re-validate the
cache when the method is *finished* and if it meets the criteria
passed to the decorator.

If getBaseRepo is called and the cache is invalid it will immediately
re-validate it -- all this does is examine repos and their enabled
attribute, this should not be a long running process.

In addition to this, in the software watcher it was possible for access
to payload.groups and payload.environments to stomp on the yum preconf
state while the PAYLOAD_MD thread was running, so wait for it to finish
as well.
---
 pyanaconda/packaging/__init__.py     | 14 +-------
 pyanaconda/packaging/dnfpayload.py   |  2 +-
 pyanaconda/packaging/yumpayload.py   | 64 +++++++++++++-----------------------
 pyanaconda/ui/gui/spokes/software.py |  6 ++--
 pyanaconda/ui/gui/spokes/source.py   | 12 +++----
 pyanaconda/ui/tui/spokes/software.py | 13 ++++----
 pyanaconda/ui/tui/spokes/source.py   |  8 ++---
 7 files changed, 42 insertions(+), 77 deletions(-)

diff --git a/pyanaconda/packaging/__init__.py b/pyanaconda/packaging/__init__.py
index a5bcb01..3686e2e 100644
--- a/pyanaconda/packaging/__init__.py
+++ b/pyanaconda/packaging/__init__.py
@@ -147,21 +147,9 @@ class Payload(object):
         """ A list of addon repo identifiers. """
         return [r.name for r in self.data.repo.dataList()]
 
-    def getBaseRepo(self, wait=True, callback=None):
+    def getBaseRepo(self):
         """
         Get the identifier of the current base repo.
-
-        :param wait: whether to block until the identifier is derived from the
-                     configuration (may take a long time) or just try to use
-                     the cached value and return REPO_NOT_SET if there is none
-        :type wait: bool
-        :param callback: callback that will be called once the indentifier is
-                         derived from the configuration (gets the repo ID as
-                         the first argument) if it is not returned instantly
-        :type callback: str -> None
-        :returns: id of the current base repo or None if wait=True is used or
-                  REPO_NOT_SET if wait=False is used and the value is not cached
-
         """
         return None
 
diff --git a/pyanaconda/packaging/dnfpayload.py b/pyanaconda/packaging/dnfpayload.py
index 3dd673c..3b78574 100644
--- a/pyanaconda/packaging/dnfpayload.py
+++ b/pyanaconda/packaging/dnfpayload.py
@@ -217,7 +217,7 @@ class DNFPayload(packaging.PackagePayload):
         # addon repos via kickstart
         return [r.name for r in self.data.repo.dataList()]
 
-    def getBaseRepo(self, wait=True, callback=None):
+    def getBaseRepo(self):
         # is any locking needed here as in the yumpayload?
         repo_names = [constants.BASE_REPO_NAME] + DEFAULT_REPOS
         for repo in self._base.repos.iter_enabled():
diff --git a/pyanaconda/packaging/yumpayload.py b/pyanaconda/packaging/yumpayload.py
index 2f2813a..9c157c3 100644
--- a/pyanaconda/packaging/yumpayload.py
+++ b/pyanaconda/packaging/yumpayload.py
@@ -62,7 +62,7 @@ except ImportError:
     log.error("import of yum failed")
     yum = None
 
-from pyanaconda.constants import BASE_REPO_NAME, DRACUT_ISODIR, INSTALL_TREE, ISO_DIR, MOUNT_DIR, ROOT_PATH, THREAD_REFRESH_BASE_REPO
+from pyanaconda.constants import BASE_REPO_NAME, DRACUT_ISODIR, INSTALL_TREE, ISO_DIR, MOUNT_DIR, ROOT_PATH
 from pyanaconda.flags import flags
 
 from pyanaconda import iutil
@@ -78,9 +78,7 @@ from pyanaconda.errors import ERROR_RAISE, errorHandler
 from pyanaconda.packaging import DependencyError, MetadataError, NoNetworkError, NoSuchGroup, \
                                  NoSuchPackage, PackagePayload, PayloadError, PayloadInstallError, \
                                  PayloadSetupError
-from pyanaconda.packaging import REPO_NOT_SET
 from pyanaconda.progress import progressQ
-from pyanaconda.threads import threadMgr, AnacondaThread
 
 from pyanaconda.localization import langcode_matches_locale
 
@@ -117,13 +115,13 @@ class YumLock(object):
 def invalidates_br_cache(cond_fn=None):
     """
     Function returning decorator for methods that invalidate base repo cache.
+    After the method has been run the base repo will be re-validated
 
     :param cond_fn: condition function telling if base repo should be
                     invalidated or not (HAS TO TAKE THE SAME ARGUMENTS AS THE
                     DECORATED FUNCTION)
 
     """
-
     def decorator(method):
         """
         Decorator for methods that invalidate base repo cache.
@@ -131,13 +129,14 @@ def invalidates_br_cache(cond_fn=None):
         :param method: decorated method of the YumPayload class
 
         """
-
         @wraps(method)
         def inner_method(yum_payload, *args, **kwargs):
+            ret = method(yum_payload, *args, **kwargs)
+
             if cond_fn is None or cond_fn(yum_payload, *args, **kwargs):
                 yum_payload.invalidate_br_cache()
-
-            return method(yum_payload, *args, **kwargs)
+                yum_payload._refreshBaseRepo()
+            return ret
 
         return inner_method
 
@@ -175,9 +174,7 @@ class YumPayload(PackagePayload):
         # base repo caching
         self._cached_base_repo = None
         self._br_cache_valid = False
-        self._br_callbacks = []
-        self._br_cache_valid_lock = threading.Lock()
-        self._br_refresh_lock = threading.Lock()
+        self._br_cache_valid_lock = threading.RLock()
 
         self.reset()
 
@@ -422,29 +419,21 @@ reposdir=%s
 
         return _repos
 
-    def getBaseRepo(self, wait=True, callback=None):
-        # prevent potential thread calling callbacks from finishing if we want
-        # to add some more
-        with self._br_refresh_lock:
-            # test and set atomically
-            with self._br_cache_valid_lock:
-                if self._br_cache_valid:
-                    return self._cached_base_repo
-
-            # cache not valid, should we start a thread to refresh it?
-            refresh_thread = threadMgr.get(THREAD_REFRESH_BASE_REPO)
-            if not refresh_thread:
-                threadMgr.add(AnacondaThread(name=THREAD_REFRESH_BASE_REPO, target=self._refreshBaseRepo))
-
-            if callback:
-                self._br_callbacks.append(callback)
-
-        if wait:
-            threadMgr.wait(THREAD_REFRESH_BASE_REPO)
-            return self._cached_base_repo
-        else:
-            # not waiting for the thread and cache was not valid
-            return REPO_NOT_SET
+    def getBaseRepo(self):
+        """ Return the cached base repo id, if it is valid.
+            Otherwise, re-validate it and return it.
+
+            This should result in minmal access to _yum_lock, especially
+            since the methods that would invalidate the cached value also
+            re-validate it after they have been run. These methods are
+            decorated with @invalidate_br_cache
+        """
+        with self._br_cache_valid_lock:
+            if self._br_cache_valid:
+                return self._cached_base_repo
+
+        self._refreshBaseRepo()
+        return self._cached_base_repo
 
     def _refreshBaseRepo(self):
         with _yum_lock:
@@ -455,7 +444,6 @@ reposdir=%s
                         # cache the value for multiple use
                         self._cached_base_repo = repo_name
                         self._br_cache_valid = True
-                        current_cached = self._cached_base_repo
                     break
             else:
                 # didn't find any base repo set and enabled
@@ -463,14 +451,6 @@ reposdir=%s
                     # set the cache to None, but make it valid
                     self._cached_base_repo = None
                     self._br_cache_valid = True
-                    current_cached = self._cached_base_repo
-
-        with self._br_refresh_lock:
-            # go through scheduled callbacks (if any) and remove them
-            for callback in self._br_callbacks:
-                callback(current_cached)
-
-            self._br_callbacks = []
 
     @property
     def mirrorEnabled(self):
diff --git a/pyanaconda/ui/gui/spokes/software.py b/pyanaconda/ui/gui/spokes/software.py
index ce2089e..0e1fa66 100644
--- a/pyanaconda/ui/gui/spokes/software.py
+++ b/pyanaconda/ui/gui/spokes/software.py
@@ -193,12 +193,10 @@ class SoftwareSelectionSpoke(NormalSpoke):
 
         hubQ.send_message(self.__class__.__name__, _("Downloading group metadata..."))
 
+        threadMgr.wait(constants.THREAD_PAYLOAD_MD)
         # we have no way to select environments with kickstart right now
         # so don't try.
-        if flags.automatedInstall and self.data.packages.seen:
-            # We don't want to do a full refresh, just join the metadata thread
-            threadMgr.wait(constants.THREAD_PAYLOAD_MD)
-        else:
+        if not (flags.automatedInstall and self.data.packages.seen):
             # Grabbing the list of groups could potentially take a long time
             # at first (yum does a lot of magic property stuff, some of which
             # involves side effects like network access.  We need to reference
diff --git a/pyanaconda/ui/gui/spokes/source.py b/pyanaconda/ui/gui/spokes/source.py
index e93e313..ffbf7d7 100644
--- a/pyanaconda/ui/gui/spokes/source.py
+++ b/pyanaconda/ui/gui/spokes/source.py
@@ -348,7 +348,7 @@ class SourceSpoke(NormalSpoke):
         elif self._mirror_active():
             # this preserves the url for later editing
             self.data.method.method = None
-            if not old_source.method and self.payload.getBaseRepo(wait=True) and \
+            if not old_source.method and self.payload.getBaseRepo() and \
                not self._proxyChange:
                 return False
         elif self._http_active() or self._ftp_active():
@@ -450,7 +450,7 @@ class SourceSpoke(NormalSpoke):
             hubQ.send_message(self.__class__.__name__, _(METADATA_DOWNLOAD_MESSAGE))
             self.payload.gatherRepoMetadata()
             self.payload.release()
-            if not self.payload.getBaseRepo(wait=True):
+            if not self.payload.getBaseRepo():
                 hubQ.send_message(self.__class__.__name__, _(METADATA_ERROR_MESSAGE))
                 hubQ.send_ready(self.__class__.__name__, False)
                 self._error = True
@@ -482,10 +482,10 @@ class SourceSpoke(NormalSpoke):
 
     @property
     def completed(self):
-        if flags.automatedInstall and (not self.data.method.method or not self.payload.getBaseRepo(wait=False)):
+        if flags.automatedInstall and (not self.data.method.method or not self.payload.getBaseRepo()):
             return False
         else:
-            return not self._error and self.ready and (self.data.method.method or self.payload.getBaseRepo(wait=False))
+            return not self._error and self.ready and (self.data.method.method or self.payload.getBaseRepo())
 
     @property
     def mandatory(self):
@@ -504,7 +504,7 @@ class SourceSpoke(NormalSpoke):
             return _("Checking software dependencies...")
         elif not self.ready:
             return _(BASEREPO_SETUP_MESSAGE)
-        elif not self.payload.getBaseRepo(wait=True):
+        elif not self.payload.getBaseRepo():
             return _("Error setting up base repository")
         elif self._error:
             return _("Error setting up software source")
@@ -518,7 +518,7 @@ class SourceSpoke(NormalSpoke):
             if not self._currentIsoFile:
                 return _("Error setting up ISO file")
             return os.path.basename(self._currentIsoFile)
-        elif self.payload.getBaseRepo(wait=True):
+        elif self.payload.getBaseRepo():
             return _("Closest mirror")
         else:
             return _("Nothing selected")
diff --git a/pyanaconda/ui/tui/spokes/software.py b/pyanaconda/ui/tui/spokes/software.py
index 087cc5b..18c3689 100644
--- a/pyanaconda/ui/tui/spokes/software.py
+++ b/pyanaconda/ui/tui/spokes/software.py
@@ -60,9 +60,8 @@ class SoftwareSpoke(NormalTUISpoke):
     def _initialize(self):
         """ Private initialize. """
         threadMgr.wait(THREAD_PAYLOAD)
-        if self._kickstarted:
-            threadMgr.wait(THREAD_PAYLOAD_MD)
-        else:
+        threadMgr.wait(THREAD_PAYLOAD_MD)
+        if not self._kickstarted:
             try:
                 self.payload.environments
             except MetadataError:
@@ -79,7 +78,7 @@ class SoftwareSpoke(NormalTUISpoke):
             return _("Error checking software selection")
         if not self.ready:
             return _("Processing...")
-        if not self.payload.getBaseRepo(wait=False):
+        if not self.payload.getBaseRepo():
             return _("Installation source not set up")
         if not self.txid_valid:
             return _("Source changed - please verify")
@@ -109,15 +108,15 @@ class SoftwareSpoke(NormalTUISpoke):
                          not self.errors and self.txid_valid
 
         if flags.automatedInstall:
-            return processingDone and self.payload.getBaseRepo(wait=False) and self.data.packages.seen
+            return processingDone and self.payload.getBaseRepo() and self.data.packages.seen
         else:
-            return self.payload.getBaseRepo(wait=False) and self.environment is not None and processingDone
+            return self.payload.getBaseRepo() and self.environment is not None and processingDone
 
     def refresh(self, args=None):
         """ Refresh screen. """
         NormalTUISpoke.refresh(self, args)
 
-        if not self.payload.getBaseRepo(wait=False):
+        if not self.payload.getBaseRepo():
             message = TextWidget(_("Installation source needs to be set up first."))
             self._window.append(message)
 
diff --git a/pyanaconda/ui/tui/spokes/source.py b/pyanaconda/ui/tui/spokes/source.py
index d167ee9..565dc0f 100644
--- a/pyanaconda/ui/tui/spokes/source.py
+++ b/pyanaconda/ui/tui/spokes/source.py
@@ -193,7 +193,7 @@ class SourceSpoke(SourceSwitchHandler, EditTUISpoke):
             if not self.data.method.dir:
                 return _("Error setting up software source")
             return os.path.basename(self.data.method.dir)
-        elif self.payload.getBaseRepo(wait=True):
+        elif self.payload.getBaseRepo():
             return _("Closest mirror")
         else:
             return _("Nothing selected")
@@ -218,10 +218,10 @@ class SourceSpoke(SourceSwitchHandler, EditTUISpoke):
 
     @property
     def completed(self):
-        if flags.automatedInstall and (not self.data.method.method or not self.payload.getBaseRepo(wait=False)):
+        if flags.automatedInstall and (not self.data.method.method or not self.payload.getBaseRepo()):
             return False
         else:
-            return not self.errors and self.ready and (self.data.method.method or self.payload.getBaseRepo(wait=False))
+            return not self.errors and self.ready and (self.data.method.method or self.payload.getBaseRepo())
 
     def refresh(self, args=None):
         EditTUISpoke.refresh(self, args)
@@ -316,7 +316,7 @@ class SourceSpoke(SourceSwitchHandler, EditTUISpoke):
         else:
             self.payload.gatherRepoMetadata()
             self.payload.release()
-            if not self.payload.getBaseRepo(wait=True):
+            if not self.payload.getBaseRepo():
                 self.errors.append(_("Error downloading package metadata"))
             else:
                 try:
-- 
1.8.3.1



More information about the anaconda-patches mailing list