From 87fd48eafee12842b3a08066145a1cdaf5f43643 Mon Sep 17 00:00:00 2001 From: Fabio Erculiani Date: Tue, 20 Mar 2012 07:58:13 +0100 Subject: [PATCH] [Rigo] introduce SharedLocker to avoid reentrancy of Entropy Resources Lock There are two issues with Entropy Resources Lock when used in Rigo. 1. Reentrancy: this property is unwanted in Rigo due to the amount of time Entropy Resources Lock is acquired and released. It is always wanted to acquire once and release once. 2. Re-acquiring it is costly, this lock should be relased only when we are really forced to do so. --- rigo/rigo_app.py | 126 +++++++++++++++++++++++++++++------------------ 1 file changed, 77 insertions(+), 49 deletions(-) diff --git a/rigo/rigo_app.py b/rigo/rigo_app.py index d14045780..3bdc0543e 100644 --- a/rigo/rigo_app.py +++ b/rigo/rigo_app.py @@ -93,6 +93,40 @@ class RigoServiceController(GObject.Object): message_type=message_type, context_id=RigoServiceController.NOTIFICATION_CONTEXT_ID) + class SharedLocker(object): + + """ + SharedLocker ensures that Entropy Resources + lock and unlock operations are called once, + avoiding reentrancy, which is a property of + lock_resources() and unlock_resources(), even + during concurrent access. + """ + + def __init__(self, entropy_client, locked): + self._entropy = entropy_client + self._locking_mutex = Lock() + self._locked = locked + + def lock(self): + with self._locking_mutex: + lock = False + if not self._locked: + lock = True + self._locked = True + if lock: + self._entropy.lock_resources( + blocking=True, shared=True) + + def unlock(self): + with self._locking_mutex: + unlock = False + if self._locked: + unlock = True + self._locked = False + if unlock: + self._entropy.unlock_resources() + __gsignals__ = { # we request to lock the whole UI wrt repo # interaction @@ -213,9 +247,8 @@ class RigoServiceController(GObject.Object): # since we handle the lock/unlock of entropy # resources here, we need to know what's the # initial state - self._resources_locked = Lock() - if not shared_locked: - self._resources_locked.acquire() + self._shared_locker = self.SharedLocker( + self._entropy, shared_locked) def service_available(self): """ @@ -570,9 +603,7 @@ class RigoServiceController(GObject.Object): "_resources_lock_request_signal._resources_lock: " "enter (sleep)") - self._entropy.lock_resources( - blocking=True, - shared=True) + self._shared_locker.lock() self._release_local_resources() const_debug_write( @@ -580,17 +611,10 @@ class RigoServiceController(GObject.Object): "_resources_lock_request_signal._resources_lock: " "regained shared lock") - # it's a muted, ask the MainLoop to release it - # or it will explode - GLib.idle_add(self._resources_locked.release) - - # check if we actually released them - locked = self._resources_locked.acquire(False) - if not locked: - task = ParallelTask(_resources_lock) - task.name = "ResourceLockAfterRelease" - task.daemon = True - task.start() + task = ParallelTask(_resources_lock) + task.name = "ResourceLockAfterRelease" + task.daemon = True + task.start() def _resources_unlock_request_signal(self, activity): """ @@ -603,32 +627,30 @@ class RigoServiceController(GObject.Object): "_resources_unlock_request_signal: " "called, with remote activity: %s" % (activity,)) - # FIXME: there might be a race here between local_activity - # and subsequent busy() called by _update_repositories - # wrt unlock_resources() and locking back. - # maybe we should busy() here and then call ParallelTask? - # OR: we just need to lock_resources() back in case of - # error activity switch not being accepted. - if activity == DaemonActivityStates.UPDATING_REPOSITORIES: # did we ask that or is it another client? local_activity = self.local_activity() if local_activity == LocalActivityStates.READY: - locked = self._resources_locked.acquire(False) - if locked: - self._entropy.unlock_resources() + + def _update_repositories(): + accepted = self._update_repositories( + [], False, master=False) + if accepted: + const_debug_write( + __name__, + "_resources_unlock_request_signal: " + "_update_repositories accepted, unlocking") + self._shared_locker.unlock() # another client, bend over XD # LocalActivityStates value will be atomically # switched in the above thread. - task = ParallelTask( - self._update_repositories, - [], False, - master=False) + task = ParallelTask(_update_repositories) task.daemon = True task.name = "UpdateRepositoriesExternal" task.start() + const_debug_write( __name__, "_resources_unlock_request_signal: " @@ -636,9 +658,7 @@ class RigoServiceController(GObject.Object): elif local_activity == \ LocalActivityStates.UPDATING_REPOSITORIES: - locked = self._resources_locked.acquire(False) - if locked: - self._entropy.unlock_resources() + self._shared_locker.unlock() const_debug_write( __name__, @@ -658,20 +678,25 @@ class RigoServiceController(GObject.Object): local_activity = self.local_activity() if local_activity == LocalActivityStates.READY: - locked = self._resources_locked.acquire(False) - if locked: - self._entropy.unlock_resources() + + def _application_request(): + accepted = self._application_request( + None, None, master=False) + if accepted: + const_debug_write( + __name__, + "_resources_unlock_request_signal: " + "_application_request accepted, unlocking") + self._shared_locker.unlock() # another client, bend over XD # LocalActivityStates value will be atomically # switched in the above thread. - task = ParallelTask( - self._application_request, - None, None, - master=False) + task = ParallelTask(_application_request) task.daemon = True task.name = "ApplicationRequestExternal" task.start() + const_debug_write( __name__, "_resources_unlock_request_signal: " @@ -679,9 +704,7 @@ class RigoServiceController(GObject.Object): elif local_activity == \ LocalActivityStates.MANAGING_APPLICATIONS: - locked = self._resources_locked.acquire(False) - if locked: - self._entropy.unlock_resources() + self._shared_locker.unlock() const_debug_write( __name__, @@ -888,13 +911,13 @@ class RigoServiceController(GObject.Object): "LocalActivityStates.BusyError!") # 1 -- ACTIVITY CRIT :: OFF self._activity_rwsem.writer_release() - return + return False except LocalActivityStates.SameError: const_debug_write(__name__, "_update_repositories: " "LocalActivityStates.SameError!") # 1 -- ACTIVITY CRIT :: OFF self._activity_rwsem.writer_release() - return + return False if master: scaled = self._scale_up( @@ -903,7 +926,7 @@ class RigoServiceController(GObject.Object): self.unbusy(local_activity) # 1 -- ACTIVITY CRIT :: OFF self._activity_rwsem.writer_release() - return + return False accepted = self._update_repositories_unlocked( repositories, force, master) @@ -920,6 +943,9 @@ class RigoServiceController(GObject.Object): box.add_destroy_button(_("K thanks")) self._nc.append(box) GLib.idle_add(_notify) + return False + + return True def _update_repositories_unlocked(self, repositories, force, master): @@ -1133,7 +1159,7 @@ class RigoServiceController(GObject.Object): const_debug_write(__name__, "_application_request: " "LocalActivityStates.BusyError!") # doing other stuff, cannot go ahead - return + return False except LocalActivityStates.SameError: const_debug_write(__name__, "_application_request: " "LocalActivityStates.SameError, " @@ -1160,7 +1186,7 @@ class RigoServiceController(GObject.Object): DaemonActivityStates.MANAGING_APPLICATIONS) if not scaled: _unbusy() - return + return False # clean terminal, make sure no crap is left there if self._terminal is not None: @@ -1191,6 +1217,8 @@ class RigoServiceController(GObject.Object): self._nc.append(box) GLib.idle_add(_notify) + return accepted + class WorkViewController(GObject.Object):