[master/f20 1/3] Remove threading from getBaseRepo handling (#1011555)

Brian C. Lane bcl at redhat.com
Thu Nov 21 19:01:18 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.
---
 pyanaconda/constants.py              |  1 -
 pyanaconda/packaging/__init__.py     | 14 +-------
 pyanaconda/packaging/dnfpayload.py   |  2 +-
 pyanaconda/packaging/yumpayload.py   | 64 +++++++++++++-----------------------
 pyanaconda/ui/gui/spokes/source.py   | 12 +++----
 pyanaconda/ui/tui/spokes/software.py |  9 ++---
 pyanaconda/ui/tui/spokes/source.py   |  8 ++---
 7 files changed, 39 insertions(+), 71 deletions(-)

diff --git a/pyanaconda/constants.py b/pyanaconda/constants.py
index 115ef28..7af5014 100644
--- a/pyanaconda/constants.py
+++ b/pyanaconda/constants.py
@@ -108,7 +108,6 @@ THREAD_GEOLOCATION_REFRESH = "AnaGeolocationRefreshThread"
 THREAD_DATE_TIME = "AnaDateTimeThread"
 THREAD_TIME_INIT = "AnaTimeInitThread"
 THREAD_XKL_WRAPPER_INIT = "AnaXklWrapperInitThread"
-THREAD_REFRESH_BASE_REPO = "AnaBaseRepoRefreshThread"
 
 # Geolocation constants
 
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 9de7ceb..7e5cd41 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 c82fc94..a01f8c3 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/source.py b/pyanaconda/ui/gui/spokes/source.py
index e1e05ad..d1963b6 100644
--- a/pyanaconda/ui/gui/spokes/source.py
+++ b/pyanaconda/ui/gui/spokes/source.py
@@ -350,7 +350,7 @@ class SourceSpoke(NormalSpoke):
             # this preserves the url for later editing
             self.data.method.method = None
             self.data.method.proxy = self._proxyUrl
-            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():
@@ -457,7 +457,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
@@ -489,10 +489,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):
@@ -511,7 +511,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")
@@ -525,7 +525,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..87eaefc 100644
--- a/pyanaconda/ui/tui/spokes/software.py
+++ b/pyanaconda/ui/tui/spokes/software.py
@@ -60,6 +60,7 @@ class SoftwareSpoke(NormalTUISpoke):
     def _initialize(self):
         """ Private initialize. """
         threadMgr.wait(THREAD_PAYLOAD)
+
         if self._kickstarted:
             threadMgr.wait(THREAD_PAYLOAD_MD)
         else:
@@ -79,7 +80,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 +110,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