Check if reentrant version of CFITSIO is used #5239
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
szkarpinski
force-pushed
the
fits-reentrant
branch
from
533a412
to
d26b271
Compare
|
CI MESSAGE: [11485028]: BUILD STARTED |
|
CI MESSAGE: [11485028]: BUILD PASSED |
awolant
approved these changes
Dec 15, 2023
mzient
reviewed
Dec 15, 2023
Comment on lines
61
to
65
|
|
||
| DALI_ENFORCE(fits_is_reentrant(), | ||
| "Loaded instance of cfitsio library does not support multithreading. " | ||
| "Please recompile cfitsio in reentrant mode (--enable-reentrant) " | ||
| "or use cfitsio delivered in DALI_deps"); |
There was a problem hiding this comment.
Couldn't we just warn the user and guard the code with a mutex, like this?
std::unique_lock lock(mtx, std::defer_lock);
if (!fits_is_reentrant)
lock.lock();...unless we statically link it and ship it with the .whl.
There was a problem hiding this comment.
Good idea! Thank you!
There was a problem hiding this comment.
I added locking to FITS_CALL
mzient
reviewed
Jan 8, 2024
DALI_DEPS_VERSION
Outdated
| @@ -1 +1 @@ | |||
| 345c534814ffcfd40c2df3fe3d4a00c0d911b94c | |||
| FIXME | |||
There was a problem hiding this comment.
Revert to what's in main - the relevant DALI_deps PR has already been merged.
Signed-off-by: Szymon Karpiński <skarpinski@nvidia.com>
Signed-off-by: Szymon Karpiński <skarpinski@nvidia.com>
Signed-off-by: Szymon Karpiński <skarpinski@nvidia.com>
szkarpinski
force-pushed
the
fits-reentrant
branch
from
9b9e70a
to
57af13a
Compare
mzient
approved these changes
Jan 8, 2024
|
!build |
|
CI MESSAGE: [11957948]: BUILD STARTED |
|
CI MESSAGE: [11957948]: BUILD PASSED |
mzient
pushed a commit
to mzient/DALI
that referenced
this pull request
Jan 9, 2024
Check if reentrant version of CFITSIO is used. If not, emit a warning and use exclusive lock when calling CFITSIO functions. --------- Signed-off-by: Szymon Karpiński <skarpinski@nvidia.com>
mzient
pushed a commit
to mzient/DALI
that referenced
this pull request
Jan 9, 2024
Check if reentrant version of CFITSIO is used. If not, emit a warning and use exclusive lock when calling CFITSIO functions. --------- Signed-off-by: Szymon Karpiński <skarpinski@nvidia.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Category:
Bug fix (non-breaking change which fixes an issue)
Description:
CFITSIO is by default compiled in non-reentrant mode, resulting in segfaults when two or more pipelines are active. NVIDIA/DALI_deps#96 enables multithreading and this PR adds a runtime check and a test for that.
Additional information:
Affected modules and functionalities:
Key points relevant for the review:
Tests:
Checklist
Documentation
DALI team only
Requirements
REQ IDs: N/A
JIRA TASK: DALI-3746