Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BUG: sparse.linalg: add locks to ensure ARPACK threadsafety #5848

Merged
merged 2 commits into from
Feb 17, 2016

Conversation

pv
Copy link
Member

@pv pv commented Feb 15, 2016

The ARPACK fortran code uses SAVE variables, and is not threadsafe or
re-entrant. Add locks and reentrancy checks to the Python side to deal
with this issue --- previously the code used to segfault.

As we use an explicit lock, it is not necessary for the Fortran code to
hold the GIL, so adjust the .pyf file accordingly.

Fixes gh-5846

The ARPACK fortran code uses SAVE variables, and is not threadsafe or
re-entrant. Add locks and reentrancy checks to the Python side to deal
with this issue --- previously the code used to segfault.

As we use an explicit lock, it is not necessary for the Fortran code to
hold the GIL, so adjust the .pyf file accordingly.

Fixes scipygh-5846
@codecov-io
Copy link

@@            master   #5848   diff @@
======================================
  Files          236     237     +1
  Stmts        43344   43374    +30
  Branches      8134    8135     +1
  Methods          0       0       
======================================
+ Hit          33803   33832    +29
- Partial       2585    2586     +1
  Missed        6956    6956       

Review entire Coverage Diff as of c7028b7

Powered by Codecov. Updated on successful CI builds.

@ewmoore
Copy link
Member

ewmoore commented Feb 16, 2016

LGTM as is.

However, this will mean that we have two slightly different versions of reentrancy check code in sparse.linalg alone. The version that this adds and the existing code in isolve/iterative.py. This seems like a sign that maybe this should be factored out somewhere. _lib maybe?

@pv
Copy link
Member Author

pv commented Feb 16, 2016

refactored

@@ -42,6 +42,8 @@

__all__ = ['eigs', 'eigsh', 'svds', 'ArpackError', 'ArpackNoConvergence']

import contextlib
import threading
Copy link
Member

Choose a reason for hiding this comment

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

These imports aren't necessary anymore I don't think.

@pv
Copy link
Member Author

pv commented Feb 17, 2016

removed unnecessary imports

ev-br added a commit that referenced this pull request Feb 17, 2016
BUG: sparse.linalg: add locks to ensure ARPACK threadsafety
@ev-br ev-br merged commit a6c2a72 into scipy:master Feb 17, 2016
@ev-br
Copy link
Member

ev-br commented Feb 17, 2016

Thanks, merged.

@ev-br ev-br added defect A clear bug or issue that prevents SciPy from being installed or used as expected scipy.sparse.linalg labels Feb 17, 2016
@ev-br ev-br added this to the 0.18.0 milestone Feb 17, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
defect A clear bug or issue that prevents SciPy from being installed or used as expected scipy.sparse.linalg
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants