Opened 11 years ago

Closed 11 years ago

#12002 closed defect (fixed)

avoid race conditions when creating directories - scripts

Reported by: Jeroen Demeyer Owned by: Leif Leonhardy
Priority: blocker Milestone: sage-4.8
Component: scripts Keywords:
Cc: John Palmieri Merged in: sage-4.8.alpha2
Authors: Jeroen Demeyer Reviewers: John Palmieri
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by Jeroen Demeyer)

Same as #11972 but for the "scripts" repository.

Directories should be created using

try:
    os.makedirs(PATH)
except OSError:
    if not os.path.isdir(PATH): 
        raise

Attachments (1)

12002.patch (3.6 KB) - added by Jeroen Demeyer 11 years ago.

Download all attachments as: .zip

Change History (11)

comment:1 Changed 11 years ago by Jeroen Demeyer

Description: modified (diff)

comment:2 Changed 11 years ago by Jeroen Demeyer

Authors: Jeroen Demeyer
Status: newneeds_review

comment:3 Changed 11 years ago by Jeroen Demeyer

Priority: majorblocker

comment:4 Changed 11 years ago by John Palmieri

In sage-ptest, the code

    if os.path.isdir(TMP):  # used to be os.path.exists(TMP)
        shutil.rmtree(TMP)
    os.makedirs(TMP)

will now fail if TMP exists but is not a directory. Is this okay? I wouldn't mind just deleting TMP regardless, in which case the code should probably be

try:
    shutil.rmtree(TMP)
except ....

or perhaps

shutil.rmtree(tmp, ignore_errors=True)

Otherwise it looks okay to me.

comment:5 in reply to:  4 Changed 11 years ago by Jeroen Demeyer

Replying to jhpalmieri:

In sage-ptest, the code

    if os.path.isdir(TMP):  # used to be os.path.exists(TMP)
        shutil.rmtree(TMP)
    os.makedirs(TMP)

will now fail if TMP exists but is not a directory.

My idea was that TMP should never be an ordinary file. If it ever happens that TMP is a file, then something must have gone badly wrong.

comment:6 Changed 11 years ago by John Palmieri

We still have the possibility of a race condition, though:

  if os.path.isdir(TMP):   # suppose TMP does not exist at this point
      shutil.rmtree(TMP)
  # but suppose by now TMP exists
  os.makedirs(TMP)

Instead maybe this?

  try:
     os.makedirs(TMP)
  except OSError:
     if os.path.isdir(TMP):
        shutil.rmtree(TMP)
        os.makedirs(TMP)
     else:
        raise

comment:7 in reply to:  6 Changed 11 years ago by Jeroen Demeyer

Status: needs_reviewneeds_work

Replying to jhpalmieri:

We still have the possibility of a race condition, though:

  if os.path.isdir(TMP):   # suppose TMP does not exist at this point
      shutil.rmtree(TMP)
  # but suppose by now TMP exists
  os.makedirs(TMP)

Instead maybe this?

  try:
     os.makedirs(TMP)
  except OSError:
     if os.path.isdir(TMP):
        shutil.rmtree(TMP)
        os.makedirs(TMP)
     else:
        raise

There is still a race condition (between the rmtree and makedirs) but I don't think this can be avoided here. But I like your code, using a nice try/except block.

Changed 11 years ago by Jeroen Demeyer

Attachment: 12002.patch added

comment:8 Changed 11 years ago by Jeroen Demeyer

Reviewers: John Palmieri
Status: needs_workneeds_review

comment:9 Changed 11 years ago by John Palmieri

Status: needs_reviewpositive_review

Looks good to me. See #12018 for a sort of follow-up, or at least an issue that I noticed because of this ticket.

comment:10 Changed 11 years ago by Jeroen Demeyer

Merged in: sage-4.8.alpha2
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.