From 9eb995670cdaaeb6b92c6b37db652363efd1b7fa Mon Sep 17 00:00:00 2001 From: Lea Waller Date: Thu, 29 Jul 2021 19:43:27 +0200 Subject: [PATCH 1/5] Also allow `errno.EBUSY` during `emptydirs` on NFS - Can occur if a file is still open somewhere, so NFS will rename it to a hidden file in the same directory - When `shutil` tries to delete that hidden file, we get an `OSError` with `errno.EBUSY` --- nipype/utils/filemanip.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/nipype/utils/filemanip.py b/nipype/utils/filemanip.py index f02efa163f..e009608b77 100644 --- a/nipype/utils/filemanip.py +++ b/nipype/utils/filemanip.py @@ -781,7 +781,7 @@ def emptydirs(path, noexist_ok=False): shutil.rmtree(path) except OSError as ex: elcont = os.listdir(path) - if ex.errno == errno.ENOTEMPTY and not elcont: + if ex.errno in [errno.ENOTEMPTY, errno.EBUSY] and not elcont: fmlogger.warning( "An exception was raised trying to remove old %s, but the path" " seems empty. Is it an NFS mount?. Passing the exception.", From 7a0803f9483d7e743fb9a5379fbb65cb478b2b87 Mon Sep 17 00:00:00 2001 From: Lea Waller Date: Thu, 29 Jul 2021 20:07:24 +0200 Subject: [PATCH 2/5] Ignore `.nfs` placeholder files when catching the error - I forgot that `os.listdir` also lists hidden files in the previous commit --- nipype/utils/filemanip.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/nipype/utils/filemanip.py b/nipype/utils/filemanip.py index e009608b77..f51e35c6b3 100644 --- a/nipype/utils/filemanip.py +++ b/nipype/utils/filemanip.py @@ -780,7 +780,7 @@ def emptydirs(path, noexist_ok=False): try: shutil.rmtree(path) except OSError as ex: - elcont = os.listdir(path) + elcont = [p for p in os.listdir(path) if not p.startswith(".nfs")] if ex.errno in [errno.ENOTEMPTY, errno.EBUSY] and not elcont: fmlogger.warning( "An exception was raised trying to remove old %s, but the path" From 6fec48bad244248362c19900c7bc291719136bd6 Mon Sep 17 00:00:00 2001 From: Lea Waller Date: Fri, 30 Jul 2021 07:30:39 +0200 Subject: [PATCH 3/5] Add unit test for `emptydirs` on NFS - With mock for NFS silly-rename (yes, it's really called that) behavior --- nipype/utils/filemanip.py | 8 ++++++-- nipype/utils/tests/test_filemanip.py | 19 +++++++++++++++++++ 2 files changed, 25 insertions(+), 2 deletions(-) diff --git a/nipype/utils/filemanip.py b/nipype/utils/filemanip.py index f51e35c6b3..0a2c71f9db 100644 --- a/nipype/utils/filemanip.py +++ b/nipype/utils/filemanip.py @@ -780,7 +780,11 @@ def emptydirs(path, noexist_ok=False): try: shutil.rmtree(path) except OSError as ex: - elcont = [p for p in os.listdir(path) if not p.startswith(".nfs")] + elcont = [ + Path(root) / file + for root, _, files in os.walk(path) for file in files + if not file.startswith(".nfs") + ] if ex.errno in [errno.ENOTEMPTY, errno.EBUSY] and not elcont: fmlogger.warning( "An exception was raised trying to remove old %s, but the path" @@ -793,7 +797,7 @@ def emptydirs(path, noexist_ok=False): else: raise ex - os.makedirs(path) + os.makedirs(path, exist_ok=True) def silentrm(filename): diff --git a/nipype/utils/tests/test_filemanip.py b/nipype/utils/tests/test_filemanip.py index 299029a8d2..211c8b2bc3 100644 --- a/nipype/utils/tests/test_filemanip.py +++ b/nipype/utils/tests/test_filemanip.py @@ -31,6 +31,7 @@ savepkl, path_resolve, write_rst_list, + emptydirs, ) @@ -670,3 +671,21 @@ def test_write_rst_list(tmp_path, items, expected): else: with pytest.raises(expected): write_rst_list(items) + + +def nfs_unlink(pathlike, *, dir_fd=None): + os.rename(pathlike, ".nfs1111111111", src_dir_fd=dir_fd, dst_dir_fd=dir_fd) + + +def test_emptydirs_dangling_nfs(tmp_path): + busyfile = tmp_path / "base" / "subdir" / "busyfile" + busyfile.parent.mkdir(parents=True) + busyfile.touch() + + with mock.patch("os.unlink") as mocked: + mocked.side_effect = nfs_unlink + emptydirs(tmp_path / "base") + + assert Path.exists(tmp_path / "base") + assert not busyfile.exists() + assert busyfile.parent.exists() # Couldn't remove From 05aa5540116b83afeb573104d9f3ba7c8d267464 Mon Sep 17 00:00:00 2001 From: Lea Waller Date: Fri, 30 Jul 2021 07:50:01 +0200 Subject: [PATCH 4/5] Run `black` --- nipype/utils/filemanip.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/nipype/utils/filemanip.py b/nipype/utils/filemanip.py index 0a2c71f9db..03786ec935 100644 --- a/nipype/utils/filemanip.py +++ b/nipype/utils/filemanip.py @@ -782,7 +782,8 @@ def emptydirs(path, noexist_ok=False): except OSError as ex: elcont = [ Path(root) / file - for root, _, files in os.walk(path) for file in files + for root, _, files in os.walk(path) + for file in files if not file.startswith(".nfs") ] if ex.errno in [errno.ENOTEMPTY, errno.EBUSY] and not elcont: From eafdb9334f8ca12e67298306156e9bb2cbb2c21a Mon Sep 17 00:00:00 2001 From: Lea Waller Date: Fri, 30 Jul 2021 12:53:46 +0200 Subject: [PATCH 5/5] Update nipype/utils/tests/test_filemanip.py Handle mock test case when no `dir_fd` is passed --- nipype/utils/tests/test_filemanip.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/nipype/utils/tests/test_filemanip.py b/nipype/utils/tests/test_filemanip.py index 211c8b2bc3..f02ad4164e 100644 --- a/nipype/utils/tests/test_filemanip.py +++ b/nipype/utils/tests/test_filemanip.py @@ -674,7 +674,12 @@ def test_write_rst_list(tmp_path, items, expected): def nfs_unlink(pathlike, *, dir_fd=None): - os.rename(pathlike, ".nfs1111111111", src_dir_fd=dir_fd, dst_dir_fd=dir_fd) + if dir_fd is None: + path = Path(pathlike) + deleted = path.with_name(".nfs00000000") + path.rename(deleted) + else: + os.rename(pathlike, ".nfs1111111111", src_dir_fd=dir_fd, dst_dir_fd=dir_fd) def test_emptydirs_dangling_nfs(tmp_path):