Skip to content

Make leap seconds threadsafe#108

Open
ColmTalbot wants to merge 7 commits intoliberfa:masterfrom
ColmTalbot:make-leap-seconds-threadsafe
Open

Make leap seconds threadsafe#108
ColmTalbot wants to merge 7 commits intoliberfa:masterfrom
ColmTalbot:make-leap-seconds-threadsafe

Conversation

@ColmTalbot
Copy link
Copy Markdown

These are changes to enable use with freethreaded Python builds.
The only real change is making the leap second table (and count) thread local.
The logic for identifying how to find the appropriate identifier came from ChatGPT and I'm open to other suggestions.
This works with the OSs used in the CI and there is a fallback to the old behaviour.

See liberfa/pyerfa#209, cc @neutrinoceros @mhvk

I didn't implement the suggestion in #103 to preinitialize the leap second array.
It worked fine on mac/linux, but failed on windows (see this CI job).

@neutrinoceros
Copy link
Copy Markdown
Contributor

From afar, this approach seems sound to me, with the important limitation that the problem is entirely new to me and I don't know the internals of liberfa. I will defer any actual review to Marten.

Copy link
Copy Markdown
Contributor

@mhvk mhvk left a comment

Choose a reason for hiding this comment

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

This seems like a good change, but see my request for a comment about provenance in-line. In the meantime, I'll start the tests, just to check things do not break on something more obscure.

** For thread safety we want a way to define thread-local variables.
** If no TLS is available, fall back to ordinary storage and disable
** thread safety for those variables.
*/
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It would be good to include in the comment where these definitions come from, in case things break later and we have to update or so. I notice that they are somewhat different from what is used by numpy

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I hadn't seen the numpy version, thanks for linking.

The differences in the conditions are that I missed C23 also has thread_local rather than _Thread_local and for cpp there is an explicit check of the version being > = C++11. I extended the comment to include more of this.

The main difference I see is the main checks are added in meson.build.
I tested this with erfa and it wasn't clear where this should go, I think for safety it would need to be in meson.build, configure.ac, and setup.py.
I'm happy to push a version with that, but it is quite a lot of repetition.
Do you think that would be better?

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants