#10159 closed defect (fixed)
matplotlib: avoid race conditions when creating (config) directories
Reported by: | jhpalmieri | Owned by: | tbd |
---|---|---|---|
Priority: | major | Milestone: | sage-4.6.1 |
Component: | packages: standard | Keywords: | matplotlib MPLCONFIGDIR tex.cache condition errno 17 |
Cc: | Merged in: | sage-4.6.1.alpha0 | |
Authors: | John Palmieri | Reviewers: | Leif Leonhardy |
Report Upstream: | Reported upstream. Little or no feedback. | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description
This is a followup to #6235. You can find a new spkg at
http://sage.math.washington.edu/home/palmieri/SPKG/matplotlib-1.0.0.p0.spkg
and I've attached the mercurial patch from the old version to the new, for reference. There are two changes: record in the SPKG.txt file the change from #6235 about MPLCONFIGDIR, so that in future upgrades, we don't screw up how that variable is set. Also, patch texmanager.py so that it creates the config dir in a way less likely to cause problems. (See #8677 for a similar situation; the solution was taken from there.)
Attachments (1)
Change History (13)
comment:1 Changed 10 years ago by
- Status changed from new to needs_review
comment:2 Changed 10 years ago by
- Report Upstream changed from N/A to Reported upstream. Little or no feedback.
I just reported this upstream. If I get any feedback, I'll report it here.
comment:3 follow-up: ↓ 4 Changed 10 years ago by
- Reviewers set to Leif Leonhardy
- Status changed from needs_review to positive_review
Apart from the typo in the commit message, and I'm not sure if I like assert
there, I'm ok with the patch, and the spkg is clean.
Works for me as advertised, so positive review.
(Tested on Ubuntu 10.04 x86_64, Core2 Quad. I also deleted the MPL config dir and then ran
$ ./sage -tp N -long devel/sage/sage/plot/ # N={32,64} (though only 49 files)
Each time all doctests passed. Perhaps others should stress-[doc]test it on other platforms like MacOS X, too.)
comment:4 in reply to: ↑ 3 Changed 10 years ago by
comment:5 Changed 10 years ago by
Oh, I just noticed the patch does not (exactly) what the ticket advertises... (In fact, the patch addresses the race condition in the creation of the TeX cache.)
There are still some other instances of if not os.path.exists(foo): mkdir(foo)
and similar, so it's a bit surprising I did not run into any race condition.
John, are you willing to fix these, too?
Unless you meanwhile have Sun's grep installed, you can find them by e.g.
$ grep -B5 -A2 mkdir `find src -name \*.py`
(Giving about two false positives; also not all are equally important I think.)
comment:6 Changed 10 years ago by
I tried the same multiple times with DOT_SAGE
set to some NFS-mounted filesystem, made that busy by another machine, but still don't get any errors...
comment:7 Changed 10 years ago by
Okay, I've fixed some more of these. I think that only the files in src/lib/matplotlib get installed, so I haven't bothered with the other ones. I also ignored the function "mkdirs" in cbook.py. This is only used in sphinxext/plot_directive.py, so I don't think it should affect us. Also, the function could perhaps just be replaced with a call to os.makedirs, which is already recursive and allows you to set the mode. Maybe the matplotlib people are worried about the presence of ".." in the pathname? Anyway, I don't have a good way to test any changes to that part of the code, so I'm going to leave it alone.
comment:8 Changed 10 years ago by
- Status changed from positive_review to needs_work
comment:9 Changed 10 years ago by
- Status changed from needs_work to needs_review
comment:10 Changed 10 years ago by
- Keywords MPLCONFIGDIR tex.cache condition errno 17 added
- Status changed from needs_review to positive_review
- Summary changed from matplotlib: avoid race condition when creating config directory to matplotlib: avoid race conditions when creating (config) directories
Ok.
Also, raise
is more adequate there, since one cannot assert the only possible OSError
is EEXIST
, so it's less confusing now.
(I still can't force errors.)
comment:11 Changed 10 years ago by
- Merged in set to sage-4.6.1.alpha0
- Resolution set to fixed
- Status changed from positive_review to closed
comment:12 Changed 10 years ago by
- Milestone changed from sage-4.6 to sage-4.6.1
A note about the patch: it is a little large because I had to add the file patches/texmanager.py to the repository. Also, in addition to the important changes, it also changes various parts of SPKG.txt so that the lines are shorter than 80 characters.