Opened 12 years ago

Closed 12 years ago

Last modified 12 years ago

#10159 closed defect (fixed)

matplotlib: avoid race conditions when creating (config) directories

Reported by: John Palmieri 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:

Status badges

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)

trac_10159-matplotlib.patch (90.3 KB) - added by John Palmieri 12 years ago.
Mercurial patch for matplotlib spkg; for reference only

Download all attachments as: .zip

Change History (13)

comment:1 Changed 12 years ago by John Palmieri

Status: newneeds_review

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.

comment:2 Changed 12 years ago by John Palmieri

Report Upstream: N/AReported upstream. Little or no feedback.

I just reported this upstream. If I get any feedback, I'll report it here.

comment:3 Changed 12 years ago by Leif Leonhardy

Reviewers: Leif Leonhardy
Status: needs_reviewpositive_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 12 years ago by John Palmieri

Replying to leif:

Apart from the typo in the commit message

If you mean the spurious apostrophe, I've removed it.

, and I'm not sure if I like assert

I just took that from #8677.

Works for me as advertised, so positive review.

Great, thanks!

comment:5 Changed 12 years ago by Leif Leonhardy

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 12 years ago by Leif Leonhardy

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 12 years ago by John Palmieri

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 12 years ago by John Palmieri

Status: positive_reviewneeds_work

comment:9 Changed 12 years ago by John Palmieri

Status: needs_workneeds_review

Changed 12 years ago by John Palmieri

Attachment: trac_10159-matplotlib.patch added

Mercurial patch for matplotlib spkg; for reference only

comment:10 Changed 12 years ago by Leif Leonhardy

Keywords: MPLCONFIGDIR tex.cache condition errno 17 added
Status: needs_reviewpositive_review
Summary: matplotlib: avoid race condition when creating config directorymatplotlib: 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 12 years ago by Jeroen Demeyer

Merged in: sage-4.6.1.alpha0
Resolution: fixed
Status: positive_reviewclosed

comment:12 Changed 12 years ago by Mitesh Patel

Milestone: sage-4.6sage-4.6.1
Note: See TracTickets for help on using tickets.