Skip to content

Commit c9c4734

Browse files
committed
Make sure we never context switch while holding VM lock.
We were seeing errors in our application that looked like: ``` [BUG] unexpected situation - recordd:1 current:0 /error.c:1097 rb_bug_without_die_internal /vm_sync.c:275 disallow_reentry /eval_intern.h:136 rb_ec_vm_lock_rec_check /eval_intern.h:147 rb_ec_tag_state /vm.c:2619 rb_vm_exec /vm.c:1702 rb_yield /eval.c:1173 rb_ensure ``` We concluded that there was context switching going on while a thread held the VM lock. During the investigation into the issue, we added assertions that we never yield to another thread with the VM lock held. We enabled these VM lock assertions even in single ractor mode. These assertions were failing in a few places, but most notably in finalizers. We were running finalizers with the VM lock held, and they were context switching and causing this issue. These rules must be held going forward to ensure we don't context switch unexpectedly: If you have the VM lock held, * Don't enter the interpreter loop. * Don't yield to ruby code. * Don't call rb_nogvl (it will context switch you and will not unlock the VM lock). * Don't check your own interrupts, it can switch you. If you don't have the GVL: * Don't call rb_ensure/rb_protect, etc (these are old rules but good to have assertions for).
1 parent 8f040a5 commit c9c4734

File tree

16 files changed

+117
-45
lines changed

16 files changed

+117
-45
lines changed

encoding.c

Lines changed: 23 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -717,34 +717,35 @@ rb_encdb_alias(const char *alias, const char *orig)
717717
static void
718718
rb_enc_init(struct enc_table *enc_table)
719719
{
720-
ASSERT_vm_locking();
721-
enc_table_expand(enc_table, ENCODING_COUNT + 1);
722-
if (!enc_table->names) {
723-
enc_table->names = st_init_strcasetable_with_size(ENCODING_LIST_CAPA);
724-
}
720+
RB_VM_LOCKING() {
721+
enc_table_expand(enc_table, ENCODING_COUNT + 1);
722+
if (!enc_table->names) {
723+
enc_table->names = st_init_strcasetable_with_size(ENCODING_LIST_CAPA);
724+
}
725725
#define OnigEncodingASCII_8BIT OnigEncodingASCII
726726
#define ENC_REGISTER(enc) enc_register_at(enc_table, ENCINDEX_##enc, rb_enc_name(&OnigEncoding##enc), &OnigEncoding##enc)
727-
ENC_REGISTER(ASCII_8BIT);
728-
ENC_REGISTER(UTF_8);
729-
ENC_REGISTER(US_ASCII);
730-
global_enc_ascii = enc_table->list[ENCINDEX_ASCII_8BIT].enc;
731-
global_enc_utf_8 = enc_table->list[ENCINDEX_UTF_8].enc;
732-
global_enc_us_ascii = enc_table->list[ENCINDEX_US_ASCII].enc;
727+
ENC_REGISTER(ASCII_8BIT);
728+
ENC_REGISTER(UTF_8);
729+
ENC_REGISTER(US_ASCII);
730+
global_enc_ascii = enc_table->list[ENCINDEX_ASCII_8BIT].enc;
731+
global_enc_utf_8 = enc_table->list[ENCINDEX_UTF_8].enc;
732+
global_enc_us_ascii = enc_table->list[ENCINDEX_US_ASCII].enc;
733733
#undef ENC_REGISTER
734734
#undef OnigEncodingASCII_8BIT
735735
#define ENCDB_REGISTER(name, enc) enc_register_at(enc_table, ENCINDEX_##enc, name, NULL)
736-
ENCDB_REGISTER("UTF-16BE", UTF_16BE);
737-
ENCDB_REGISTER("UTF-16LE", UTF_16LE);
738-
ENCDB_REGISTER("UTF-32BE", UTF_32BE);
739-
ENCDB_REGISTER("UTF-32LE", UTF_32LE);
740-
ENCDB_REGISTER("UTF-16", UTF_16);
741-
ENCDB_REGISTER("UTF-32", UTF_32);
742-
ENCDB_REGISTER("UTF8-MAC", UTF8_MAC);
743-
744-
ENCDB_REGISTER("EUC-JP", EUC_JP);
745-
ENCDB_REGISTER("Windows-31J", Windows_31J);
736+
ENCDB_REGISTER("UTF-16BE", UTF_16BE);
737+
ENCDB_REGISTER("UTF-16LE", UTF_16LE);
738+
ENCDB_REGISTER("UTF-32BE", UTF_32BE);
739+
ENCDB_REGISTER("UTF-32LE", UTF_32LE);
740+
ENCDB_REGISTER("UTF-16", UTF_16);
741+
ENCDB_REGISTER("UTF-32", UTF_32);
742+
ENCDB_REGISTER("UTF8-MAC", UTF8_MAC);
743+
744+
ENCDB_REGISTER("EUC-JP", EUC_JP);
745+
ENCDB_REGISTER("Windows-31J", Windows_31J);
746746
#undef ENCDB_REGISTER
747-
enc_table->count = ENCINDEX_BUILTIN_MAX;
747+
enc_table->count = ENCINDEX_BUILTIN_MAX;
748+
}
748749
}
749750

750751
rb_encoding *

eval_intern.h

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -95,7 +95,10 @@ extern int select_large_fdset(int, fd_set *, fd_set *, fd_set *, struct timeval
9595

9696
#include <sys/stat.h>
9797

98+
int ruby_thread_has_gvl_p(void);
99+
98100
#define EC_PUSH_TAG(ec) do { \
101+
VM_ASSERT(ruby_thread_has_gvl_p()); \
99102
rb_execution_context_t * const _ec = (ec); \
100103
struct rb_vm_tag _tag; \
101104
_tag.state = TAG_NONE; \

gc/default/default.c

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2773,7 +2773,7 @@ rb_gc_impl_define_finalizer(void *objspace_ptr, VALUE obj, VALUE block)
27732773

27742774
for (i = 0; i < len; i++) {
27752775
VALUE recv = RARRAY_AREF(table, i);
2776-
if (rb_equal(recv, block)) {
2776+
if (rb_equal(recv, block)) { // TODO: unsafe, can context switch
27772777
RB_GC_VM_UNLOCK(lev);
27782778
return recv;
27792779
}
@@ -2839,8 +2839,8 @@ get_final(long i, void *data)
28392839
return RARRAY_AREF(table, i + 1);
28402840
}
28412841

2842-
static void
2843-
run_final(rb_objspace_t *objspace, VALUE zombie)
2842+
static int
2843+
run_final(rb_objspace_t *objspace, VALUE zombie, unsigned int lev)
28442844
{
28452845
if (RZOMBIE(zombie)->dfree) {
28462846
RZOMBIE(zombie)->dfree(RZOMBIE(zombie)->data);
@@ -2851,7 +2851,9 @@ run_final(rb_objspace_t *objspace, VALUE zombie)
28512851
FL_UNSET(zombie, FL_FINALIZE);
28522852
st_data_t table;
28532853
if (st_delete(finalizer_table, &key, &table)) {
2854+
RB_GC_VM_UNLOCK(lev);
28542855
rb_gc_run_obj_finalizer(RARRAY_AREF(table, 0), RARRAY_LEN(table) - 1, get_final, (void *)table);
2856+
lev = RB_GC_VM_LOCK();
28552857
}
28562858
else {
28572859
rb_bug("FL_FINALIZE flag is set, but finalizers are not found");
@@ -2860,6 +2862,7 @@ run_final(rb_objspace_t *objspace, VALUE zombie)
28602862
else {
28612863
GC_ASSERT(!st_lookup(finalizer_table, key, NULL));
28622864
}
2865+
return lev;
28632866
}
28642867

28652868
static void
@@ -2874,7 +2877,7 @@ finalize_list(rb_objspace_t *objspace, VALUE zombie)
28742877

28752878
int lev = RB_GC_VM_LOCK();
28762879

2877-
run_final(objspace, zombie);
2880+
lev = run_final(objspace, zombie, lev);
28782881
{
28792882
GC_ASSERT(BUILTIN_TYPE(zombie) == T_ZOMBIE);
28802883
GC_ASSERT(page->heap->final_slots_count > 0);

io.c

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14432,7 +14432,13 @@ argf_filename(VALUE argf)
1443214432
static VALUE
1443314433
argf_filename_getter(ID id, VALUE *var)
1443414434
{
14435-
return argf_filename(*var);
14435+
VALUE filename = Qnil;
14436+
RB_VM_UNLOCK();
14437+
{
14438+
filename = argf_filename(*var);
14439+
}
14440+
RB_VM_LOCK();
14441+
return filename;
1443614442
}
1443714443

1443814444
/*

ractor.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -164,7 +164,7 @@ ractor_status_set(rb_ractor_t *r, enum ractor_status status)
164164
// check 1
165165
if (r->status_ != ractor_created) {
166166
VM_ASSERT(r == GET_RACTOR()); // only self-modification is allowed.
167-
ASSERT_vm_locking();
167+
VM_ASSERT((rb_multi_ractor_p() && RB_VM_LOCKED_P()) || true);
168168
}
169169

170170
// check2: transition check. assume it will be vanished on non-debug build.

string.c

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12707,7 +12707,9 @@ Init_String(void)
1270712707
{
1270812708
rb_cString = rb_define_class("String", rb_cObject);
1270912709

12710-
rb_concurrent_set_foreach_with_replace(fstring_table_obj, fstring_set_class_i, NULL);
12710+
RB_VM_LOCKING() {
12711+
rb_concurrent_set_foreach_with_replace(fstring_table_obj, fstring_set_class_i, NULL);
12712+
}
1271112713

1271212714
rb_include_module(rb_cString, rb_mComparable);
1271312715
rb_define_alloc_func(rb_cString, empty_str_alloc);

symbol.c

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -390,7 +390,9 @@ rb_free_global_symbol_table_i(VALUE *sym_ptr, void *data)
390390
void
391391
rb_free_global_symbol_table(void)
392392
{
393-
rb_concurrent_set_foreach_with_replace(ruby_global_symbols.sym_set, rb_free_global_symbol_table_i, NULL);
393+
RB_VM_LOCKING() {
394+
rb_concurrent_set_foreach_with_replace(ruby_global_symbols.sym_set, rb_free_global_symbol_table_i, NULL);
395+
}
394396
}
395397

396398
WARN_UNUSED_RESULT(static ID lookup_str_id(VALUE str));

thread.c

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1472,6 +1472,7 @@ rb_thread_sleep(int sec)
14721472
static void
14731473
rb_thread_schedule_limits(uint32_t limits_us)
14741474
{
1475+
VM_ASSERT(GET_VM()->ractor.sync.lock_owner != GET_RACTOR());
14751476
if (!rb_thread_alone()) {
14761477
rb_thread_t *th = GET_THREAD();
14771478
RUBY_DEBUG_LOG("us:%u", (unsigned int)limits_us);

thread_pthread.c

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1025,6 +1025,7 @@ thread_sched_to_waiting_common(struct rb_thread_sched *sched, rb_thread_t *th)
10251025
static void
10261026
thread_sched_to_waiting(struct rb_thread_sched *sched, rb_thread_t *th)
10271027
{
1028+
VM_ASSERT(GET_VM()->ractor.sync.lock_owner != GET_RACTOR()); // never context switch with VM lock held
10281029
thread_sched_lock(sched, th);
10291030
{
10301031
thread_sched_to_waiting_common(sched, th);

transcode.c

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -412,10 +412,11 @@ int rb_require_internal_silent(VALUE fname);
412412
static const rb_transcoder *
413413
load_transcoder_entry(transcoder_entry_t *entry)
414414
{
415-
ASSERT_vm_unlocking();
416415
if (entry->transcoder)
417416
return entry->transcoder;
418417

418+
ASSERT_vm_unlocking();
419+
419420
if (entry->lib) {
420421
const char *const lib = entry->lib;
421422
const size_t len = strlen(lib);
@@ -1854,7 +1855,13 @@ rb_econv_asciicompat_encoding(const char *ascii_incompat_name)
18541855
RB_VM_LOCK_ENTER_LEV(&lev);
18551856
}
18561857
else {
1858+
#if RUBY_DEBUG > 0
1859+
RB_VM_LOCK_LEAVE_LEV(&lev);
1860+
#endif
18571861
st_foreach(table2, asciicompat_encoding_i, (st_data_t)&data);
1862+
#if RUBY_DEBUG > 0
1863+
RB_VM_LOCK_ENTER_LEV(&lev);
1864+
#endif
18581865
}
18591866
}
18601867

0 commit comments

Comments
 (0)