Get String#crypt working with multi-ractor in cases where !HAVE_CRYPT_R #13567
+8
−1
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dc39cd1 to
8d25d6f
Compare
In commit 12f7ba5, ractor safety was added to String#crypt, however in certain cases it can cause a deadlock. When we lock a native mutex, we cannot allocate ruby objects because they might trigger GC which starts a VM barrier. If the barrier is triggered and other native threads are waiting on this mutex, they will not be able to be woken up in order to join the barrier. To fix this, we don't allocate ruby objects when we hold the lock. The following could reproduce the problem: ```ruby strings = [] 10_000.times do |i| strings << "my string #{i}" end STRINGS = Ractor.make_shareable(strings) rs = [] 100.times do rs << Ractor.new do STRINGS.each do |s| s.dup.crypt(s.dup) end end end while rs.any? r, obj = Ractor.select(*rs) rs.delete(r) end ``` I will not be adding tests because I am almost finished a PR to enable running test-all test cases inside many ractors at once, which is how I found the issue. Co-authored-by: jhawthorn <john@hawthorn.email>
8d25d6f to
eb97984
Compare
jhawthorn
reviewed
Jun 10, 2025
Comment on lines
+11271
to
+11273
| size_t res_size = strlen(res)+1; | ||
| char *dup = malloc(res_size); // need to hold onto lock while duplicating a potentially static buffer | ||
| memcpy(dup, res, res_size); |
There was a problem hiding this comment.
Note for readers: We have to do this because of our #define strdup ruby_strdup. We want to malloc here, not xmalloc. And I don't think there's a portable way to determine the size of crypt's output buffer.
There was a problem hiding this comment.
res = (strdup)(res); can avoid the macro.
jhawthorn
reviewed
Jun 10, 2025
| // Don't allocate a ruby object while holding this lock, we could hit a VM barrier, which | ||
| // causes a deadlock if other ractors are waiting on this lock. | ||
| result = rb_str_new_cstr(dup); | ||
| free(dup); |
There was a problem hiding this comment.
If rb_str_new_cstr erorrs, this pointer might leak.
I think that may not be worth dealing with as crypt is semi-deprecated https://bugs.ruby-lang.org/issues/14915
I prefer that (very unlikely) leak to the deadlock that will occur in the test suite under ractors.
jhawthorn
approved these changes
Jun 10, 2025
nobu
reviewed
Jun 10, 2025
| result = rb_str_new_cstr(res); | ||
|
|
||
| size_t res_size = strlen(res)+1; | ||
| char *dup = malloc(res_size); // need to hold onto lock while duplicating a potentially static buffer |
There was a problem hiding this comment.
The encrypted result would be small enough in alloca.
And this copy should not be needed if crypt_r is available.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In commit 12f7ba5, ractor safety was added to String#crypt, however in certain cases it can cause a deadlock. When we lock a native mutex, we cannot allocate ruby objects because they might trigger GC which starts a VM barrier. If the barrier is triggered and other native threads are waiting on this mutex, they will not be able to be woken up in order to join the barrier. To fix this, we don't allocate ruby objects when we hold the lock.
The following could reproduce the problem:
I will not be adding tests because I am almost finished a PR to enable running test-all test cases inside many ractors at once, which is how I found the issue.