Opened 18 months ago

Closed 17 months ago

Last modified 4 weeks ago

#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:

Status badges

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 soehms

Branch: u/soehms/atomic_dir_32344

comment:2 Changed 18 months ago by soehms

Authors: Sebastian Oehms
Commit: 681cf4d950d64c003fae988954b94fdc64c6b27f
Status: newneeds_review

New commits:

681cf4d32344: initial version

comment:3 Changed 18 months ago by mkoeppe

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 in reply to:  3 Changed 18 months ago by soehms

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 Changed 18 months ago by 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

comment:6 Changed 18 months ago by git

Commit: 681cf4d950d64c003fae988954b94fdc64c6b27f6d4693d580d0d5f87dba4f8b6d795d25ba7f109e

Branch pushed to git repo; I updated commit sha1. New commits:

00b7106Merge branch 'u/soehms/atomic_dir_32344' of git://trac.sagemath.org/sage into atomic_dir_32344
6d4693d32344: keep dir if it exists

comment:7 in reply to:  5 Changed 18 months ago by soehms

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 mkoeppe

Milestone: sage-9.4sage-9.5

comment:9 Changed 17 months ago by mkoeppe

Reviewers: Matthias Koeppe
Status: needs_reviewpositive_review

LGTM

comment:10 Changed 17 months ago by soehms

Many thanks!

comment:11 Changed 17 months ago by vbraun

Branch: u/soehms/atomic_dir_323446d4693d580d0d5f87dba4f8b6d795d25ba7f109e
Resolution: fixed
Status: positive_reviewclosed

comment:12 Changed 4 weeks ago by chapoton

Commit: 6d4693d580d0d5f87dba4f8b6d795d25ba7f109e
Keywords: temporary added; tempory removed
Note: See TracTickets for help on using tickets.