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

Fix rust send sync #259

Merged
merged 3 commits into from
Dec 23, 2024
Merged

Fix rust send sync #259

merged 3 commits into from
Dec 23, 2024

Conversation

aseyboldt
Copy link
Contributor

Fixes #258 by explicitly marking the StanLibrary as Send + Sync.

This also adds an entry to the github workflow, which should automatically find most backward incompatible changes that break semver.

@aseyboldt
Copy link
Contributor Author

aseyboldt commented Dec 23, 2024

I think I found the reason (or part of it), why rust thought this wasn't threadsafe.
The library contains version numbers as int, not const int, so two threads could modify the version number concurrently in an unsafe way.
This only showed up now, because the version numbers were not exposed to rust in the old brindgen version, but with 0.71 they are.
I changed the versions to const, and I think this should now automatically make StanLibrary Send + Sync, but there seems to be a bindgen issue (rust-lang/rust-bindgen#3059), and with this it is still exported as a mut int.
So I left the explicit Send + Sync implementation in. Since the wrapper never modifies the version (that would be fun) I think this is fine.

@WardBrian WardBrian closed this Dec 23, 2024
@WardBrian WardBrian reopened this Dec 23, 2024
@WardBrian
Copy link
Collaborator

(Sorry, fat thumbs!)

Good catch, and I think this is the correct fix.

We can release a 2.6.1 to patch this. The merge of #257 makes this a bit annoying, since we probably don’t want to change the Julia LTS in a minor version. We can try to rig up something to skip releasing Julia for this patch or I can do some cherry picking on another branch after the holidays

@WardBrian WardBrian merged commit 116f436 into roualdes:main Dec 23, 2024
38 checks passed
@aseyboldt
Copy link
Contributor Author

Sounds good, thanks.
And (slightly early) Merry Christmas :-)

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.

Rust threadsafety annotation changed in release 2.6.0
2 participants