From c2467c1208901a88c641e2b9dd21bfe3228ce0e3 Mon Sep 17 00:00:00 2001 From: Peter Silva Date: Wed, 8 Jan 2025 15:22:35 -0500 Subject: [PATCH 1/2] review and improve error handling when invoking makedirs --- sarracenia/flow/__init__.py | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/sarracenia/flow/__init__.py b/sarracenia/flow/__init__.py index 3e4e62f95..e090dac89 100644 --- a/sarracenia/flow/__init__.py +++ b/sarracenia/flow/__init__.py @@ -1654,10 +1654,13 @@ def mkdir(self, msg) -> bool: if not os.path.isdir(msg['new_dir']): try: + if os.path.exists(msg['new_dir']): + os.unlink(msg['new_dir']) os.makedirs(msg['new_dir'], self.o.permDirDefault, True) except Exception as ex: logger.warning("making %s: %s" % (msg['new_dir'], ex)) logger.debug('Exception details:', exc_info=True) + return False if os.path.isdir(path): logger.debug( f"no need to mkdir {path} as it exists" ) @@ -1703,11 +1706,15 @@ def link1file(self, msg, symbolic=True) -> bool: if not os.path.isdir(msg['new_dir']): try: + if os.path.exists(msg['new_dir']): + os.unlink(msg['new_dir']) + self.worklist.directories_ok.append(msg['new_dir']) os.makedirs(msg['new_dir'], self.o.permDirDefault, True) except Exception as ex: logger.warning("making %s: %s" % (msg['new_dir'], ex)) logger.debug('Exception details:', exc_info=True) + return False ok = True try: @@ -1768,12 +1775,17 @@ def do_download(self) -> None: if not os.path.isdir(msg['new_dir']): try: + if os.path.exists(msg['new_dir']): + os.unlink(msg['new_dir']) + logger.debug( f"missing destination directories, makedirs: {msg['new_dir']} " ) self.worklist.directories_ok.append(msg['new_dir']) os.makedirs(msg['new_dir'], 0o775, True) except Exception as ex: logger.warning("making %s: %s" % (msg['new_dir'], ex)) logger.debug('Exception details:', exc_info=True) + self.reject(msg, 422, f"cannot create directory {msg['new_dir']} to put file in it." ) + continue os.chdir(msg['new_dir']) logger.debug( f"chdir {msg['new_dir']}") From 98c55eb247d999be0064d7302b82c984abf23354 Mon Sep 17 00:00:00 2001 From: Peter Silva Date: Fri, 10 Jan 2025 09:17:11 -0500 Subject: [PATCH 2/2] unlink creates race conditions If (usual case) there are multiple files in a directory, then there is a race condition to create the directory, multiple instances will see the directory is not there, and will run makedirs to create it. the "loser" just won't create the directory... but with unlink... the unlink fails (because directory exists.) and then the operation fails. (not retried? FIXME?) So I think it's better to leave that failure mode to humans. --- sarracenia/flow/__init__.py | 8 -------- 1 file changed, 8 deletions(-) diff --git a/sarracenia/flow/__init__.py b/sarracenia/flow/__init__.py index e090dac89..d37ee4d28 100644 --- a/sarracenia/flow/__init__.py +++ b/sarracenia/flow/__init__.py @@ -1654,8 +1654,6 @@ def mkdir(self, msg) -> bool: if not os.path.isdir(msg['new_dir']): try: - if os.path.exists(msg['new_dir']): - os.unlink(msg['new_dir']) os.makedirs(msg['new_dir'], self.o.permDirDefault, True) except Exception as ex: logger.warning("making %s: %s" % (msg['new_dir'], ex)) @@ -1706,9 +1704,6 @@ def link1file(self, msg, symbolic=True) -> bool: if not os.path.isdir(msg['new_dir']): try: - if os.path.exists(msg['new_dir']): - os.unlink(msg['new_dir']) - self.worklist.directories_ok.append(msg['new_dir']) os.makedirs(msg['new_dir'], self.o.permDirDefault, True) except Exception as ex: @@ -1775,9 +1770,6 @@ def do_download(self) -> None: if not os.path.isdir(msg['new_dir']): try: - if os.path.exists(msg['new_dir']): - os.unlink(msg['new_dir']) - logger.debug( f"missing destination directories, makedirs: {msg['new_dir']} " ) self.worklist.directories_ok.append(msg['new_dir']) os.makedirs(msg['new_dir'], 0o775, True)