Use lock directory to prevent race conditions

[mdboom]

Creation of the font cache has race conditions.

Adds a dependency on lockfile. We could vendor it if we want to, but the last thing I want to do is maintain cross-platform lockfile code...

Fix #5226.

[WeatherGod]

The page for the lockfile package says that the package is deprecated and
recommends using fasteners: http://fasteners.readthedocs.org/en/latest/

On Mon, Oct 19, 2015 at 11:38 AM, Michael Droettboom <
notifications@github.com> wrote:

Creation of the font cache has race conditions.

Adds a dependency on lockfile. We could vendor it if we want to, but the
last thing I want to do is maintain cross-platform lockfile code...

Fix #5226 #5226.

You can view, comment on, or merge this pull request online at:

#5276
Commit Summary

• Use lockfile to prevent race conditions

File Changes

• M lib/matplotlib/font_manager.py
  https://github.com/matplotlib/matplotlib/pull/5276/files#diff-0 (13)
• M setup.py
  https://github.com/matplotlib/matplotlib/pull/5276/files#diff-1 (1)
• M setupext.py
  https://github.com/matplotlib/matplotlib/pull/5276/files#diff-2 (18)

Patch Links:

• https://github.com/matplotlib/matplotlib/pull/5276.patch
• https://github.com/matplotlib/matplotlib/pull/5276.diff

—
Reply to this email directly or view it on GitHub
#5276.

[mdboom]

Thanks for catching. What a pain, though. lockfile has been around forever and is well-tested (and actually has received updates more recently than fasteners) and does what we need and no more. fasteners has a lot more extra stuff -- but I guess if that's the future, that's where we are.

[mdboom]

I don't think fasteners is even the appropriate tool here. It provides for thread locks or interprocess locks, but not file system locks for processes that don't even know about each other (which is our use case). I think it's odd to recommend a package that doesn't even do the same thing. I think I'd prefer to stick with filelock here.

[WeatherGod]

Agreed. The alternative package that they recommend is oslo.concurrency
which seems pretty nice (has decorators and such), but I don't know if I
would have to install all of oslo to get just the concurrency part. Might
be a bit too heavy-handed for our needs. We can probably just stick with
lockfile for now, keeping our eyes open for an alternative later?

On Mon, Oct 19, 2015 at 11:57 AM, Michael Droettboom <
notifications@github.com> wrote:

I don't think fasteners is even the appropriate tool here. It provides for
thread locks or interprocess locks, but not file system locks for
processes that don't even know about each other (which is our use case). I
think it's odd to recommend a package that doesn't even do the same thing.
I think I'd prefer to stick with filelock here.

—
Reply to this email directly or view it on GitHub
#5276 (comment)
.

[efiring]

On 2015/10/19 5:57 AM, Michael Droettboom wrote:

I don't think fasteners is even the appropriate tool here. It provides
for /thread/ locks or /interprocess/ locks, but not file system locks
for processes that don't even know about each other (which is our use
case). I think it's odd to recommend a package that doesn't even do the
same thing. I think I'd prefer to stick with |filelock| here.

Looking at the documentation, I don't see how the interprocess lock is
restricted to processes that know about each other. One can use a
single decorator that takes a single path argument.

[mdboom]

It would seem from Travis-CI that the other problem with lockfile is that it doesn't install cleanly on Python 2.x. I'll try again with fasteners.

[pelson]

We could vendor it if we want to, but the last thing I want to do is maintain cross-platform lockfile code...

Conda have a tried and tested lockfile implementation that we could lift... https://github.com/conda/conda/blob/master/conda/lock.py - surely this must work on all their supported platforms.

[mdboom]

Thanks, Phil. That might work. My understanding is that the mkdir approach is the only cross-platform one (there are other, better options on *nix but those don't work on Windows).

[tacaswell]

Should probably change this to not say conda.

[mdboom]

Actually, I'll just take this out -- I took out the message that was displayed here anyway. I don't think the message is terribly useful in our use case.

[mdboom]

Sorry for the quality of this PR. I was working on this between millions of test runs on another PR. I keep forgetting I'm not much of a multitasking OS...

[mdboom]

Ok -- I've address the comments. I also squashed the commits, because this went on a number of divergent paths to get here...

[mdboom]

I think this is good to merge. The old hacks to get around this bug have been removed, and Travis-CI is still happy.

[jenshnielsen]

Lets see how travis behaves in the long run

[timj]

Is this patch going to make it in to the 1.5.x release? I don't see it on that branch although I may just be looking in the wrong place.

[mdboom]

It wasn't milestoned for that -- there's a fairly high probability for unintended consequences here. But I could be convinced otherwise if it gets enough testing.

[timj]

ok, we can work with the TRAVIS=1 hack in the interim.

[jenshnielsen]

Backported to 2.x as 56a932c