Add locks around accesses/modifications to global encodings table #13600
Conversation
|
Could you add a test and/or reproduction case? Was there a ticket associated with this PR? |
It tends to be difficult to make reliable reproductions to concurrency issues, and at the moment all Ractor tests need an
As Ractors are experimental and there are many issues we aren't necessarily filing tickets as the fixes will not be backported. |
|
Cool just checking, I saw this and thought neat but there was no bug associated or other context, so wasn't sure if it was ready for review or not. 🙏 |
luke-gruber
force-pushed
the
ractor_encoding_add_locks
branch
from
7792937 to
79c7b72
Compare
|
@zzak Sorry yeah, I added a test that is fairly reproducible. |
luke-gruber
force-pushed
the
ractor_encoding_add_locks
branch
3 times, most recently
from
463dbb2 to
7311646
Compare
This comment has been minimized.
This comment has been minimized.
luke-gruber
force-pushed
the
ractor_encoding_add_locks
branch
2 times, most recently
from
cffea2b to
528cb55
Compare
luke-gruber
force-pushed
the
ractor_encoding_add_locks
branch
4 times, most recently
from
ede6622 to
1a546d6
Compare
|
@jhawthorn This is ready for a review now 😅 |
luke-gruber
force-pushed
the
ractor_encoding_add_locks
branch
from
1a546d6 to
65a6473
Compare
encoding.c
Outdated
| else { | ||
| return rb_locale_encoding(); | ||
| rb_encoding *enc = NULL; | ||
| // TODO: make lock-free |
There was a problem hiding this comment.
What would it take to do this? I think we call these really a lot, and rb_enc_from_index only needs to lock in cases where the encoding is not one of the three fastpaths.
If it helps, I think it would be fine if we took a VM barrier to change internal or external encoding.
(ditto for internal encoding)
There was a problem hiding this comment.
Yeah, it's called quite a bit. I don't think it should be hard, I'll try to do it in this PR. I'll add a new commit so you can review it easily.
There was a problem hiding this comment.
@jhawthorn It's ready for review again. It turns out more than just the tbl->names stuff needs to be locked, because autoloading of encodings can happen inside ractors, which can call certain functions. However, the most-used encoding functions can still be without locks.
luke-gruber
force-pushed
the
ractor_encoding_add_locks
branch
from
57a7108 to
5cb6457
Compare
This fixes segfaults and errors of the type "Encoding not found" when
using encoding-related methods and internal encoding c functions across
ractors.
Example of a possible segfault in release mode or assertion error in debug mode:
```ruby
rs = []
100.times do
rs << Ractor.new do
"abc".force_encoding(Encoding.list.shuffle.first)
end
end
while rs.any?
r, obj = Ractor.select(*rs)
rs.delete(r)
end
```
luke-gruber
force-pushed
the
ractor_encoding_add_locks
branch
2 times, most recently
from
eb0e332 to
3974143
Compare
Also, make sure autoloading of encodings is safe across ractors.
luke-gruber
force-pushed
the
ractor_encoding_add_locks
branch
from
3974143 to
382c281
Compare
Add locks around accesses/modifications to global encodings table
This fixes segfaults and errors of the type "Encoding not found" when
using encoding-related methods and internal encoding c functions across
ractors.
Example of a possible segfault in release mode or assertion error in debug mode: