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

Avoid modifying wcsprm in-place to prevent multi-threading issues #16409

Open
astrofrog opened this issue May 7, 2024 · 2 comments
Open

Avoid modifying wcsprm in-place to prevent multi-threading issues #16409

astrofrog opened this issue May 7, 2024 · 2 comments

Comments

@astrofrog
Copy link
Member

astrofrog commented May 7, 2024

This is a different issue from:

and covers a potential issue in the astropy WCSLIB wrappers which could in theory cause multi-threading problems, though I don't have a specific example that will trigger this at this point.

In PyWcsprm, we store undefined values as NaN - however just before/after we call any WCSLIB function, we convert in-place these NaN values to UNDEFINED (a specific floating-point value) and then back afterwards. using wcsprm_python2c and wcsprm_c2python. This in-place modification isn't thread-safe, and it would be nice to find another way to do it.

Unfortunately we can't do the conversion in the getters/setters, at least not for ones returning arrays/vectors, since users might do e.g.:

wcs.wcs.cdelt[1] = 1

that is, getting the array and modifying it in-place without invoking the setter.

One possible solution would be that when we convert between NaN and undefined, we actually create temporary copies of the WCS and modify those rather than doing any in-place modification. This will double memory usage and might introduce a very small performance penalty, but both would probably have minimal impact on users and be safer. So in practice wcsprm_python2c and wcsprm_c2python would return modified copies and not operate in-place.

There might be other/better solutions though, so this issue is meant to be for discussion at this stage.

@manodeep
Copy link
Contributor

manodeep commented May 7, 2024

While the wcsprm_python2c and wcsprm_c2python have inherent thread race conditions, since they set the memory address to the same value (i.e, UNDEFINEDor NaN), there should not be any consequence of the race condition. This is unlike the code within wcs_types() where malloc/calloc is invoked and that changes the value of the variable (i..e, a new address)

@manodeep
Copy link
Contributor

manodeep commented May 7, 2024

Creating copies would create a copy per-thread and that would increase the memory requirement nthreads-fold. However, the wcsprm struct isn't that big - so perhaps the impact would not be noticeable?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants