Opened 3 years ago

Closed 3 years ago

#21599 closed defect (fixed)

Work around non-deterministic failure of uncompress on Windows

Reported by: embray Owned by:
Priority: minor Milestone: sage-7.4
Component: porting: Cygwin Keywords:
Cc: Merged in:
Authors: Erik Bray Reviewers: Jeroen Demeyer
Report Upstream: N/A Work issues:
Branch: 69830cd (Commits) Commit: 69830cd977cd6ca47d623044e3cdf9fea6f164ae
Dependencies: Stopgaps:

Description

I've noticed sometimes while building on Windows / Cygwin (though this isn't cygwin-specific) some package builds can fail randomly, usually during the uncompress step. Re-rerunning the build a second time always succeeds.

This happened one time recently and I realized there was a Python traceback leading back to the os.rename call wrapped by the attached patch.

This can happen because if any background process happens to have any file in the tree open, even briefly, the renaming the entire directory can fail. So this is especially likely to happen when unpacking source tarballs containing a large number of files.

There may be other points where this can happen, and I'll apply the same workaround if/when I encounter them.

Change History (12)

comment:1 Changed 3 years ago by embray

  • Status changed from new to needs_review

comment:2 Changed 3 years ago by jdemeyer

  • Status changed from needs_review to needs_work
  1. If the final try failed, the exception should be raised!

And then some bikeshedding:

  1. The usual way to handle such things is an exponential sleep. So I would suggest a delay *= 2 in the while loop.
  1. I would also prefer to see tries instead of retries. I find it easier to understand "3 tries" instead of "1 try and 2 retries". And you know that, because you found it necessary to explain in the docstring.

And of course this is an ugly hack, but that's not your fault.

comment:3 Changed 3 years ago by embray

D'oh! Agreed with all of the above.

comment:4 Changed 3 years ago by embray

And yes it's an ugly hack but not an uncommon one. I feel like I've done this a dozen other times for Windows support in various places.

comment:5 Changed 3 years ago by git

  • Commit changed from 21d46ab3f71017d5e982ee6c844b44c6ca2f2528 to 822f0d11c83d0f9812d435ef65a47f750820e142

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

822f0d1Re-raise the exception if it is raised on the final try.

comment:6 Changed 3 years ago by embray

  • Status changed from needs_work to needs_review

comment:7 Changed 3 years ago by jdemeyer

  • Status changed from needs_review to needs_work

Two minor things again:

  1. Replace func() by return func(). This way, you support functions which return something. It also means that you can remove the else: break.
  1. The condition while tries > 0: is now always true. So for simplicity, I would prefer to see while True there. That makes it clear that the loop can never be exited normally. By the way, this is a good example of code which would be cleaner with a goto statement.

comment:8 Changed 3 years ago by embray

Agreed again on all of the above.

The only people who think gotos are evil have never written non-trivial C code and haven't read/don't understand the context of Dijkstra's essay :)

comment:9 Changed 3 years ago by git

  • Commit changed from 822f0d11c83d0f9812d435ef65a47f750820e142 to 69830cd977cd6ca47d623044e3cdf9fea6f164ae

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

69830cdAdditional minor cleanup:

comment:10 Changed 3 years ago by embray

  • Status changed from needs_work to needs_review

New commits:

69830cdAdditional minor cleanup:
Last edited 3 years ago by embray (previous) (diff)

comment:11 Changed 3 years ago by jdemeyer

  • Reviewers set to Jeroen Demeyer
  • Status changed from needs_review to positive_review

comment:12 Changed 3 years ago by vbraun

  • Branch changed from u/embray/bug/uncompress-windows to 69830cd977cd6ca47d623044e3cdf9fea6f164ae
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.