#2715 acquire logging locks before forking
Merged a year ago by tkopecek. Opened a year ago by mikem.

file modified
+40 -1
@@ -266,6 +266,44 @@ 

          return self.state == koji.REPO_PROBLEM

  

  

+ class LoggingLockManager(object):

+     """A context manager that acquires all the logging locks"""

+ 

+     # This lock business is a workaround for https://pagure.io/koji/issue/2714

+ 

+     def __enter__(self):

+         self.acquire_locks()

+         return self

+ 

+     def __exit__(self, exc_type, exc_val, traceback):

+         # we want to free the locks regardless of what happend inside the with statement

+         self.release_locks()

+         return False  # don't eat exceptions

+ 

+     def acquire_locks(self):

+         # init

+         self.locks = []

+         self.module_lock = False

+         toplogger = logging.getLogger('koji')

+ 

+         # module level lock

+         if hasattr(logging, '_acquireLock'):

+             logging._acquireLock()

+             self.module_lock = True

+ 

+         # also each handler can have its own lock

+         # in kojira, the we should only have handlers attached to the koji logger

+         self.locks = [h.lock for h in toplogger.handlers if h.lock]

Handler.__init__ calls createLock, so ther should be no case where handler without lock is present. Am I missing some case?

+         for lock in self.locks:

+             lock.acquire()

+ 

+     def release_locks(self):

+         if self.module_lock:

+             logging._releaseLock()

+         for lock in self.locks:

+             lock.release()

+ 

+ 

  class RepoManager(object):

  

      def __init__(self, options, session):
@@ -334,7 +372,8 @@ 

          return False

  

      def _rmtree(self, path):

-         pid = os.fork()

+         with LoggingLockManager():

+             pid = os.fork()

          if pid:

              return pid

          # no return

A very simple test on my local system passes. Kojira is able to successfully delete repos.

Handler.__init__ calls createLock, so ther should be no case where handler without lock is present. Am I missing some case?

Wouldn't moving to multiprocessing.Process instead of fork() solving all these issues https://docs.python.org/3/library/multiprocessing.html#logging Maybe it would also simplify other things (we can use mp.Queue instead rm queue, etc.) But maybe it is a future improvement not needed to fix this issue.

Handler.init calls createLock, so ther should be no case where handler without lock is present. Am I missing some case?

Just trying to write defensively against possible variations in the logging lib

Wouldn't moving to multiprocessing.Process instead of fork() solving all these issues

There are definitely a few different ways to address this issue. I thought this one was the least impactful to the kojira code.

Metadata Update from @tkopecek:
- Pull-request tagged with: testing-ready

a year ago

Commit 777ce22 fixes this pull-request

Pull-Request has been merged by tkopecek

a year ago

Metadata Update from @tkopecek:
- Pull-request untagged with: testing-ready
- Pull-request tagged with: no_qe

a year ago
Metadata