Skip to content

Fix bug in locking mechanism - #9669

Merged
pllim merged 4 commits into
astropy:masterfrom
aarchiba:fix-locking-bug
Dec 4, 2019
Merged

pllim merged 4 commits into
astropy:masterfrom
aarchiba:fix-locking-bug

Conversation

@aarchiba

Copy link
Copy Markdown
Contributor

Should fix #9642

Description

This pull request is to address a bug where if the cache lock could not be acquired the failure path would incorrectly release the lock.

Fixes #9642

@astropy-bot astropy-bot Bot added the utils label Nov 24, 2019
@aarchiba

Copy link
Copy Markdown
Contributor Author

There is an unrelated problem in how I try to run different dbm backends that I have not yet tracked down. It's going to be ugly because I end up having to use monkeypatching to enforce use of a particular backend. There must be a better way.

@bsipocz bsipocz added the Bug label Nov 25, 2019
@bsipocz bsipocz added this to the v4.0.1 milestone Nov 25, 2019
@bsipocz bsipocz added the Affects-dev PRs and issues that do not impact an existing Astropy release label Nov 25, 2019

@pllim pllim left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test failures are related.

Comment thread astropy/utils/tests/test_data.py Resolved
@aarchiba

Copy link
Copy Markdown
Contributor Author

Ready for review

@pllim pllim left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know how to test this personally and it doesn't appear @mcbeth will be able to in time, so I propose that we merge this once my comment is addressed. Other issues can be separate PRs in the future as they arise.

Comment thread astropy/utils/tests/test_data.py Resolved
@pllim

pllim commented Dec 3, 2019

Copy link
Copy Markdown
Member

Depending on whether this or #9703 gets merged first, the other will need a rebase to rerun the CI before merge, just in case.

@aarchiba

aarchiba commented Dec 3, 2019

Copy link
Copy Markdown
Contributor Author

This patch actually includes test cases to evaluate what happens when the cache lock cannot be obtained, so you should be able to test it just by running the test suite.

There is one design decision I made here that one could argue with: if the cache lock cannot be obtained, raise an exception and stop. The other design option would be to proceed as if no cache were available. I chose the former because I thought it would be much easier to tell what was going on, and because the failure mode of hammering the server from all your parallel threads is not a good one.

@pllim pllim left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll trust the CI results. Thanks!

@pllim
pllim merged commit 9015e9f into astropy:master Dec 4, 2019
@pllim pllim modified the milestones: v4.0.1, v4.0 Dec 4, 2019
@aarchiba
aarchiba deleted the fix-locking-bug branch December 5, 2019 20:23
bsipocz pushed a commit that referenced this pull request Dec 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Affects-dev PRs and issues that do not impact an existing Astropy release Bug utils

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Download cache gets confused in parallel (using shell bg / GNU parallel / SLURM / etc)

3 participants