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

make bn shareable when frozen #808

Merged
merged 1 commit into from
Nov 13, 2024
Merged

Conversation

HoneyryderChuck
Copy link
Contributor

added frozen check on every state change.

ext/openssl/ossl.h Outdated Show resolved Hide resolved
ext/openssl/ossl_bn.c Outdated Show resolved Hide resolved
@rhenium
Copy link
Member

rhenium commented Oct 29, 2024

Could you make the part that changes #include <ruby/ractor.h> a separate commit or PR?

@HoneyryderChuck
Copy link
Contributor Author

@rhenium done. I'll rebase it here once it's merged.

@HoneyryderChuck
Copy link
Contributor Author

@rhenium done

Copy link
Member

@rhenium rhenium left a comment

Choose a reason for hiding this comment

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

#initialize also needs a frozen check.

Could you add a test case to ensure freezing it actually makes it shareable?

ext/openssl/ossl_bn.c Outdated Show resolved Hide resolved
@HoneyryderChuck HoneyryderChuck force-pushed the issue-521-2 branch 3 times, most recently from eab2629 to 3472aa3 Compare November 12, 2024 11:39
@HoneyryderChuck
Copy link
Contributor Author

#initialize also needs a frozen check.

why and where? 🤔

added a test.

@rhenium
Copy link
Member

rhenium commented Nov 12, 2024

ossl_bn_initialize() - it doesn't have a rb_check_frozen(self).

@HoneyryderChuck
Copy link
Contributor Author

gotcha, added. One question though: is it worth it, considering that on intialize, the object isn't expected to be frozen? What would that guard against?

@rhenium
Copy link
Member

rhenium commented Nov 13, 2024

It's needed because it's callable from Ruby, especially if the object is now expected to be thread safe when frozen. Whether calling #initialize on a frozen object makes sense or not doesn't matter.

@rhenium rhenium merged commit 2b9f444 into ruby:master Nov 13, 2024
59 checks passed
@rhenium
Copy link
Member

rhenium commented Nov 13, 2024

It looks good to me now. Thanks!

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

Successfully merging this pull request may close these issues.

2 participants