[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