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

Ensure thread-safety #284

Merged
merged 2 commits into from Jun 8, 2023
Merged

Ensure thread-safety #284

merged 2 commits into from Jun 8, 2023

Conversation

LecrisUT
Copy link
Collaborator

@LecrisUT LecrisUT commented Jun 6, 2023

Are there other static variables that need to be changed?

TODO:

  • Test if it does what it says. If it can be done locally, that would be ok as well.
    Maybe a way to do this is to spawn a few thousands workers writing random values to spglib_error_code and re-reading after a delay to confirm it's the same value.
  • Check this compiles on intel compiler

@LecrisUT LecrisUT marked this pull request as draft June 6, 2023 13:07
@LecrisUT LecrisUT requested review from atztogo and lan496 June 6, 2023 14:16
@LecrisUT
Copy link
Collaborator Author

LecrisUT commented Jun 6, 2023

@atztogo That was a quick simple fix. Added a simple tester for it as well. (try to remove _Thread_local and it should fail as expected)

@LecrisUT LecrisUT added the bug label Jun 6, 2023
@atztogo
Copy link
Collaborator

atztogo commented Jun 7, 2023

Thanks @LecrisUT. This is interesting. I didn't know about _Thread_local al all. Can it be ignored when using older standard than C11? For many years, spglib had gotten stuck roughly C89 for supporting old microsoft c compiler. Now I don't think we need to support C89, but it made me conservative, and I became to tend to be afraid of new standard. I think what at least we should have to support are the compilers used in conda-forge spglib builds. So I believe there is no issue. Do you have any comment?

@LecrisUT
Copy link
Collaborator Author

LecrisUT commented Jun 7, 2023

So there is also __thread for older gcc compilers. But I don't think it is worth keeping the old standard especially since you can't compile those compilers on currently uaed distros.

I would say a good support track is to stick to what we can check through CI, until there is an issue created for older standard support. So, a common support cycle for this is: conda-forge + containered os (currently Fedora), which usually supports up to 2 previous major releases of the compilers.

If we want an older compiler support, we can make a specific CI job for that based on Ubuntu. But we should state an EOL support for it if we do so.

Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>
Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>
@LecrisUT LecrisUT marked this pull request as ready for review June 8, 2023 08:41
@LecrisUT LecrisUT merged commit 5905a36 into spglib:develop Jun 8, 2023
10 checks passed
@LecrisUT LecrisUT deleted the thread-safety branch June 8, 2023 09:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants