#32344 closed enhancement (fixed)
Add a new class to write multi-thread safe to a complete directory
Reported by: | soehms | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-9.5 |
Component: | doctest framework | Keywords: | atomic write, directory, temporary |
Cc: | Merged in: | ||
Authors: | Sebastian Oehms | Reviewers: | Matthias Koeppe |
Report Upstream: | N/A | Work issues: | |
Branch: | 6d4693d (Commits, GitHub, GitLab) | Commit: | |
Dependencies: | Stopgaps: |
Description
This new class is just a variation of the existing class atomic_write
taking care of directory specific differences.
For a first application case see #32099.
Change History (12)
comment:1 Changed 18 months ago by
Branch: | → u/soehms/atomic_dir_32344 |
---|
comment:2 Changed 18 months ago by
Authors: | → Sebastian Oehms |
---|---|
Commit: | → 681cf4d950d64c003fae988954b94fdc64c6b27f |
Status: | new → needs_review |
comment:3 follow-up: 4 Changed 18 months ago by
Haven't tried yet, but this part:
+ except OSError: + shutil.rmtree(self.target) + os.rename(self.tempname, self.target)
looks racy to me
comment:4 Changed 18 months ago by
Replying to mkoeppe:
Haven't tried yet, but this part:
+ except OSError: + shutil.rmtree(self.target) + os.rename(self.tempname, self.target)looks racy to me
This code is just adapted from the corresponding part of atomic_write
. It worked fine for me in all of my tests (using Python 3.9). But indeed, the second patchbot (using Python 3.7) shows (ignored) FileNotFoundError
in cleanup:
Running doctests with ID 2021-08-08-00-50-13-492edc8c. Git branch: patchbot/ticket_merged Using --optional=build,ccache,debian,dochtml,pip,sage,sage_spkg Doctesting entire Sage library. Sorting sources by runtime so that slower doctests are run first.... Doctesting 4343 files using 5 threads. .... sage -t --long --random-seed=0 src/doc/en/prep/Intro-Tutorial.rst [25 tests, 1.54 s] Exception ignored in: <finalize object at 0x7fd572323bd0; dead> Traceback (most recent call last): File "/usr/lib/python3.7/weakref.py", line 552, in __call__ return info.func(*info.args, **(info.kwargs or {})) File "/usr/lib/python3.7/tempfile.py", line 934, in _cleanup _rmtree(name) File "/usr/lib/python3.7/shutil.py", line 482, in rmtree onerror(os.lstat, path, sys.exc_info()) File "/usr/lib/python3.7/shutil.py", line 480, in rmtree orig_st = os.lstat(path) FileNotFoundError: [Errno 2] No such file or directory: '/home/lelievre/.sage/temp/pascaline-sage-b/9091/tmp5ve0uefv' Exception ignored in: <finalize object at 0x7fd572323bd0; dead> Traceback (most recent call last): File "/usr/lib/python3.7/weakref.py", line 552, in __call__ return info.func(*info.args, **(info.kwargs or {})) File "/usr/lib/python3.7/tempfile.py", line 934, in _cleanup _rmtree(name) File "/usr/lib/python3.7/shutil.py", line 482, in rmtree onerror(os.lstat, path, sys.exc_info()) File "/usr/lib/python3.7/shutil.py", line 480, in rmtree orig_st = os.lstat(path) FileNotFoundError: [Errno 2] No such file or directory: '/home/lelievre/.sage/temp/pascaline-sage-b/9091/tmpgp3b5u31' Exception ignored in: <finalize object at 0x7fd572323bd0; dead> Traceback (most recent call last): File "/usr/lib/python3.7/weakref.py", line 552, in __call__ return info.func(*info.args, **(info.kwargs or {})) File "/usr/lib/python3.7/tempfile.py", line 934, in _cleanup _rmtree(name) File "/usr/lib/python3.7/shutil.py", line 482, in rmtree onerror(os.lstat, path, sys.exc_info()) File "/usr/lib/python3.7/shutil.py", line 480, in rmtree orig_st = os.lstat(path) FileNotFoundError: [Errno 2] No such file or directory: '/home/lelievre/.sage/temp/pascaline-sage-b/9091/tmpp9nuidju' sage -t --long --random-seed=0 src/sage/misc/temporary_file.py [87 tests, 1.56 s] .... ---------------------------------------------------------------------- sage -t --long --random-seed=0 src/sage/symbolic/expression.pyx # 1 doctest failed ---------------------------------------------------------------------- Total time for all tests: 2687.7 seconds cpu time: 12460.5 seconds cumulative wall time: 12941.0 seconds Pytest is not installed, skip checking tests that rely on it.
Unfortunately, the traceback doesn't show how this is caused by my code. Do you have any ideas on how to make this part more safe?
comment:5 follow-up: 7 Changed 18 months ago by
I think a first step should be to clarify the semantics in the documentation of this class: This is for creating a directory whose contents are determined uniquely by the directory name. If multiple threads or processes attempt to create it in parallel, then it does not matter which thread created it.
If this is the intended semantics, then the following should be correct:
# Success: move temporary file to target file try: os.rename(self.tempname, self.target) except FileExistsError: # Race: Another thread or process must have created the directory pass
comment:6 Changed 18 months ago by
Commit: | 681cf4d950d64c003fae988954b94fdc64c6b27f → 6d4693d580d0d5f87dba4f8b6d795d25ba7f109e |
---|
comment:7 Changed 18 months ago by
Replying to mkoeppe:
I think a first step should be to clarify the semantics in the documentation of this class: This is for creating a directory whose contents are determined uniquely by the directory name. If multiple threads or processes attempt to create it in parallel, then it does not matter which thread created it.
If this is the intended semantics, then the following should be correct:
# Success: move temporary file to target file try: os.rename(self.tempname, self.target) except FileExistsError # Race: Another thread or process must have created the directory pass
Good idea! If cleaning is necessary then the calling code is responsible. If so, it can still lead to a race of removing the directory but under better controlled conditions.
comment:8 Changed 18 months ago by
Milestone: | sage-9.4 → sage-9.5 |
---|
comment:9 Changed 17 months ago by
Reviewers: | → Matthias Koeppe |
---|---|
Status: | needs_review → positive_review |
LGTM
comment:11 Changed 17 months ago by
Branch: | u/soehms/atomic_dir_32344 → 6d4693d580d0d5f87dba4f8b6d795d25ba7f109e |
---|---|
Resolution: | → fixed |
Status: | positive_review → closed |
comment:12 Changed 4 weeks ago by
Commit: | 6d4693d580d0d5f87dba4f8b6d795d25ba7f109e |
---|---|
Keywords: | temporary added; tempory removed |
New commits:
32344: initial version