encoding.c: Acquire VM lock around `require_silent_safe` by casperisfine · Pull Request #13788 · ruby/ruby · GitHub | Latest TMZ Celebrity News & Gossip | Watch TMZ Live
Skip to content

encoding.c: Acquire VM lock around require_silent_safe #13788

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

encoding.c: Acquire VM lock around require_silent_safe #13788

wants to merge 2 commits into from
+81 −19

Conversation

casperisfine
Copy link
Contributor

require_silent_safe calls into features_index_add and a few other routines that need to be synchronized.

There are other Ractor related concerns with autoloaded encodings but this one is the biggest.

def assert(bool)
  raise "fail" unless bool
end

$-w = nil
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
assert rs.empty?

On master this fails with:

../st.c:795: Assertion Failed: rebuild_table_with:new_tab->num_entries == tab->num_entrie

    frame #5: 0x0000000100120d14 ruby`rb_assert_failure(file="../st.c", line=795, name="rebuild_table_with", expr="new_tab->num_entries == tab->num_entries") at error.c:1191:5
    frame #6: 0x000000010035c5f8 ruby`rebuild_table_with(new_tab=0x0000600000e04b40, tab=0x0000600000e125c0) at st.c:795:5
    frame #7: 0x000000010035e948 ruby`rebuild_table(tab=0x0000600000e125c0) at st.c:755:9
    frame #8: 0x00000001003596b0 ruby`rebuild_table_if_necessary(tab=0x0000600000e125c0) at st.c:1125:9
    frame #9: 0x0000000100359b2c ruby`st_add_direct_with_hash(tab=0x0000600000e125c0, key=16093565924368724757, value=15, hash=16236907536934075596) at st.c:1187:5
    frame #10: 0x000000010035ace8 ruby`rb_st_update(tab=0x0000600000e125c0, key=16093565924368724757, func=(ruby`features_index_add_single_callback at load.c:283), arg=4753754832) at st.c:1511:13
    frame #11: 0x00000001001e4d04 ruby`features_index_add_single(vm_ns=0x000000011b588e30, str="enc/encdb.bundle", len=16, offset=15, rb=false) at load.c:361:5
    frame #12: 0x00000001001e496c ruby`features_index_add(vm_ns=0x000000011b588e30, feature=4741083552, offset=15) at load.c:399:9
    frame #13: 0x00000001001e45a8 ruby`get_loaded_features_index(vm_ns=0x000000011b588e30) at load.c:455:13
    frame #14: 0x00000001001df2e0 ruby`rb_provide_feature(vm_ns=0x000000011b588e30, feature=5288623440) at load.c:754:5
    frame #15: 0x00000001001e13d0 ruby`require_internal(ec=0x000000014f842cd0, fname=5288623536, exception=1, warn=false) at load.c:1509:9
    frame #16: 0x00000001001e0bfc ruby`rb_require_internal_silent(fname=5288623536) at load.c:1527:12
    frame #17: 0x00000001000f5270 ruby`load_encoding(name="CP949") at encoding.c:746:14
    frame #18: 0x00000001000f4f80 ruby`rb_enc_autoload(enc=0x000060000293ca20) at encoding.c:797:13
    frame #19: 0x00000001000f91dc ruby`check_encoding(enc=0x000060000293ca20) at encoding.c:205:17

@launchable-app Launchable app

This comment has been minimized.

Sign in to view
@duerst
Copy link
Member

duerst commented Jul 4, 2025

Just FYI, the reason encodings are autoloaded is that most of them are very rarely used, and some of them need quite a bit of memory.

@casperisfine
Copy link
Contributor Author

@duerst I assumed as much, but thank you for confirming.

casperisfine
casperisfine commented Jul 4, 2025
View reviewed changes
load.c Outdated
@@ -372,6 +373,8 @@ features_index_add_single(vm_ns_t *vm_ns, const char* str, size_t len, VALUE off
static void
features_index_add(vm_ns_t *vm_ns, VALUE feature, VALUE offset)
{
ASSERT_vm_locking();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is failing because normally only the main ractor can call require_internal. Secondary ractors delegate require to the main one.

But this doesn't currently apply load_encoding. So the right fix would be to do that too, and the right assertion would be to check this function is only ever called in the main ractor.

@byroot
encoding.c: Acquire VM lock around require_silent_safe
6bb2b26 
`require_silent_safe` calls into `features_index_add` and a few
other routines that need to be synchronized.

There are other Ractor related concerns with autoloaded encodings
but this one is the biggest.
@byroot
Autoload encodings on the main ractor
bbc31b9 
None of the datastructures involved in the require process are
safe to call on a secondary ractor, however when autoloading
encodings, we do so from the current ractor.

So all sorts of corruption can happen when using an autoloaded
encoding for the first time from a secondary ractor.
@casperisfine
Copy link
Contributor Author

The PR now radically changed. Locking around require_silent_safe wouldn't work, because given only the main ractor is normally allowed to load code, node of the underlying data structures are synchronized.

So I went a different route, I piggy backed on rb_ractor_require to delegate the encoding load to the main ractor. This seems to pass CI, but probably need a bit of a cleanup.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@casperisfine @duerst @byroot

TMZ Celebrity News – Breaking Stories, Videos & Gossip

Looking for the latest TMZ celebrity news? You've come to the right place. From shocking Hollywood scandals to exclusive videos, TMZ delivers it all in real time.

Whether it’s a red carpet slip-up, a viral paparazzi moment, or a legal drama involving your favorite stars, TMZ news is always first to break the story. Stay in the loop with daily updates, insider tips, and jaw-dropping photos.

🎥 Watch TMZ Live

TMZ Live brings you daily celebrity news and interviews straight from the TMZ newsroom. Don’t miss a beat—watch now and see what’s trending in Hollywood.