#2755 kojira: check rm queue before adding new path
Merged a year ago by tkopecek. Opened a year ago by tkopecek.
tkopecek/koji issue2716  into  master

file modified
+4 -5
@@ -249,10 +249,8 @@ 

                  logger.error('Unable to remove volume link: %s', path)


              realpath = path

-         try:

-             self.manager.rmtree(realpath)

-         except BaseException:

-             logger.error(''.join(traceback.format_exception(*sys.exc_info())))


+         self.manager.rmtree(realpath)


          return True

@@ -340,7 +338,8 @@ 

      def rmtree(self, path):

          """Spawn (or queue) and rmtree job"""

          self.logger.info("Queuing rmtree job for %s", path)

-         self.delete_queue.append(path)

+         if path not in self.delete_queue:

+             self.delete_queue.append(path)


      def checkQueue(self):

          finished = [pid for pid in self.delete_pids if self.waitPid(pid)]

It is a remnant of previsou unification of rmtree paths. Instead of
deleting tree directly while deleting repo it is put into queue now. So,
other thread looking for expired/deleted repos can find it also and add
it twice. Internal rmtree can check the queue before adding duplicate
path. As a side-effect manager.rmtree can also never fail, so try/except
can be removed from there.

Fixes: https://pagure.io/koji/issue/2716

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

a year ago

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

a year ago

It looks like there is still a very slight chance of duplicates in a race. _pruneLocalRepos in the main thread and tryDelete in the delete thread could technically call RepoManager.rmtree for the same path at the same time. I'm not sure if this is worth worrying about since it's going to be very rare and only results in a warning.

This is probably fine for now. If it comes up, we could probably solve this potential issue by using an actual Queue object for the delete queue.

Commit 788b277 fixes this pull-request

Pull-Request has been merged by tkopecek

a year ago