Skip to content

Commit f6cbf49

Browse files
luke-gruberbyroot
authored andcommitted
Fix Symbol#to_proc (rb_sym_to_proc) to be ractor safe
In non-main ractors, don't use `sym_proc_cache`. It is not thread-safe to add to this array without a lock and also it leaks procs from one ractor to another. Instead, we create a new proc each time. If this results in poor performance we can come up with a solution later. Fixes [Bug #21354]
1 parent 97e774b commit f6cbf49

3 files changed

Lines changed: 33 additions & 15 deletions

File tree

bootstraptest/test_ractor.rb

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2307,6 +2307,18 @@ def initialize(a)
23072307
'ok'
23082308
}
23092309

2310+
# Using Symbol#to_proc inside ractors
2311+
# [Bug #21354]
2312+
assert_equal 'ok', %q{
2313+
:inspect.to_proc
2314+
Ractor.new do
2315+
# It should not use this cached proc, it should create a new one. If it used
2316+
# the cached proc, we would get a ractor_confirm_belonging error here.
2317+
:inspect.to_proc
2318+
end.take
2319+
'ok'
2320+
}
2321+
23102322
# There are some bugs in Windows with multiple threads in same ractor calling ractor actions
23112323
# Ex: https://github.com/ruby/ruby/actions/runs/14998660285/job/42139383905
23122324
unless /mswin/ =~ RUBY_PLATFORM

common.mk

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13836,6 +13836,8 @@ proc.$(OBJEXT): {$(VPATH)}prism/diagnostic.h
1383613836
proc.$(OBJEXT): {$(VPATH)}prism/version.h
1383713837
proc.$(OBJEXT): {$(VPATH)}prism_compile.h
1383813838
proc.$(OBJEXT): {$(VPATH)}proc.c
13839+
proc.$(OBJEXT): {$(VPATH)}ractor.h
13840+
proc.$(OBJEXT): {$(VPATH)}ractor_core.h
1383913841
proc.$(OBJEXT): {$(VPATH)}ruby_assert.h
1384013842
proc.$(OBJEXT): {$(VPATH)}ruby_atomic.h
1384113843
proc.$(OBJEXT): {$(VPATH)}rubyparser.h

proc.c

Lines changed: 19 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@
2222
#include "method.h"
2323
#include "iseq.h"
2424
#include "vm_core.h"
25+
#include "ractor_core.h"
2526
#include "yjit.h"
2627

2728
const rb_cref_t *rb_vm_cref_in_context(VALUE self, VALUE cbase);
@@ -1538,23 +1539,26 @@ rb_sym_to_proc(VALUE sym)
15381539
long index;
15391540
ID id;
15401541

1541-
if (!sym_proc_cache) {
1542-
sym_proc_cache = rb_ary_hidden_new(SYM_PROC_CACHE_SIZE * 2);
1543-
rb_vm_register_global_object(sym_proc_cache);
1544-
rb_ary_store(sym_proc_cache, SYM_PROC_CACHE_SIZE*2 - 1, Qnil);
1545-
}
1546-
15471542
id = SYM2ID(sym);
1548-
index = (id % SYM_PROC_CACHE_SIZE) << 1;
15491543

1550-
if (RARRAY_AREF(sym_proc_cache, index) == sym) {
1551-
return RARRAY_AREF(sym_proc_cache, index + 1);
1552-
}
1553-
else {
1554-
proc = sym_proc_new(rb_cProc, ID2SYM(id));
1555-
RARRAY_ASET(sym_proc_cache, index, sym);
1556-
RARRAY_ASET(sym_proc_cache, index + 1, proc);
1557-
return proc;
1544+
if (rb_ractor_main_p()) {
1545+
index = (id % SYM_PROC_CACHE_SIZE) << 1;
1546+
if (!sym_proc_cache) {
1547+
sym_proc_cache = rb_ary_hidden_new(SYM_PROC_CACHE_SIZE * 2);
1548+
rb_vm_register_global_object(sym_proc_cache);
1549+
rb_ary_store(sym_proc_cache, SYM_PROC_CACHE_SIZE*2 - 1, Qnil);
1550+
}
1551+
if (RARRAY_AREF(sym_proc_cache, index) == sym) {
1552+
return RARRAY_AREF(sym_proc_cache, index + 1);
1553+
}
1554+
else {
1555+
proc = sym_proc_new(rb_cProc, ID2SYM(id));
1556+
RARRAY_ASET(sym_proc_cache, index, sym);
1557+
RARRAY_ASET(sym_proc_cache, index + 1, proc);
1558+
return proc;
1559+
}
1560+
} else {
1561+
return sym_proc_new(rb_cProc, ID2SYM(id));
15581562
}
15591563
}
15601564

0 commit comments

Comments
 (0)