Opened 11 years ago

Closed 11 years ago

Last modified 11 years ago

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

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 jhpalmieri 11 years ago.
Mercurial patch for matplotlib spkg; for reference only

Download all attachments as: .zip

Change History (13)

comment:1 Changed 11 years ago by jhpalmieri

  • Status changed from new to needs_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 11 years ago by jhpalmieri

  • 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: Changed 11 years ago by leif

  • 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 11 years ago by jhpalmieri

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 11 years ago by leif

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 11 years ago by leif

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 11 years ago by jhpalmieri

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 11 years ago by jhpalmieri

  • Status changed from positive_review to needs_work

comment:9 Changed 11 years ago by jhpalmieri

  • Status changed from needs_work to needs_review

Changed 11 years ago by jhpalmieri

Mercurial patch for matplotlib spkg; for reference only

comment:10 Changed 11 years ago by leif

  • 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 11 years ago by jdemeyer

  • Merged in set to sage-4.6.1.alpha0
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:12 Changed 11 years ago by mpatel

  • Milestone changed from sage-4.6 to sage-4.6.1
Note: See TracTickets for help on using tickets.