Skip to content

Commit f8887a8

Browse files
luke-gruberluke-gruber
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). * You can still check interrupts, but we won't allow context switching 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 f8887a8

File tree

20 files changed

+233
-75
lines changed

20 files changed

+233
-75
lines changed

‎depend‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4199,7 +4199,9 @@ encoding.$(OBJEXT): $(top_srcdir)/internal/gc.h
41994199
encoding.$(OBJEXT): $(top_srcdir)/internal/imemo.h
42004200
encoding.$(OBJEXT): $(top_srcdir)/internal/inits.h
42014201
encoding.$(OBJEXT): $(top_srcdir)/internal/load.h
4202+
encoding.$(OBJEXT): $(top_srcdir)/internal/namespace.h
42024203
encoding.$(OBJEXT): $(top_srcdir)/internal/object.h
4204+
encoding.$(OBJEXT): $(top_srcdir)/internal/sanitizers.h
42034205
encoding.$(OBJEXT): $(top_srcdir)/internal/serial.h
42044206
encoding.$(OBJEXT): $(top_srcdir)/internal/set_table.h
42054207
encoding.$(OBJEXT): $(top_srcdir)/internal/static_assert.h
@@ -13362,6 +13364,7 @@ re.$(OBJEXT): {$(VPATH)}backward/2/stdalign.h
1336213364
re.$(OBJEXT): {$(VPATH)}backward/2/stdarg.h
1336313365
re.$(OBJEXT): {$(VPATH)}config.h
1336413366
re.$(OBJEXT): {$(VPATH)}constant.h
13367+
re.$(OBJEXT): {$(VPATH)}debug_counter.h
1336513368
re.$(OBJEXT): {$(VPATH)}defines.h
1336613369
re.$(OBJEXT): {$(VPATH)}encindex.h
1336713370
re.$(OBJEXT): {$(VPATH)}encoding.h
@@ -13545,6 +13548,7 @@ re.$(OBJEXT): {$(VPATH)}util.h
1354513548
re.$(OBJEXT): {$(VPATH)}vm_core.h
1354613549
re.$(OBJEXT): {$(VPATH)}vm_debug.h
1354713550
re.$(OBJEXT): {$(VPATH)}vm_opts.h
13551+
re.$(OBJEXT): {$(VPATH)}vm_sync.h
1354813552
regcomp.$(OBJEXT): $(hdrdir)/ruby.h
1354913553
regcomp.$(OBJEXT): $(hdrdir)/ruby/ruby.h
1355013554
regcomp.$(OBJEXT): {$(VPATH)}assert.h
@@ -17722,21 +17726,32 @@ time.$(OBJEXT): {$(VPATH)}timev.rbinc
1772217726
time.$(OBJEXT): {$(VPATH)}util.h
1772317727
time.$(OBJEXT): {$(VPATH)}vm_core.h
1772417728
time.$(OBJEXT): {$(VPATH)}vm_opts.h
17729+
transcode.$(OBJEXT): $(CCAN_DIR)/check_type/check_type.h
17730+
transcode.$(OBJEXT): $(CCAN_DIR)/container_of/container_of.h
17731+
transcode.$(OBJEXT): $(CCAN_DIR)/list/list.h
17732+
transcode.$(OBJEXT): $(CCAN_DIR)/str/str.h
1772517733
transcode.$(OBJEXT): $(hdrdir)/ruby/ruby.h
1772617734
transcode.$(OBJEXT): $(top_srcdir)/internal/array.h
17735+
transcode.$(OBJEXT): $(top_srcdir)/internal/basic_operators.h
1772717736
transcode.$(OBJEXT): $(top_srcdir)/internal/class.h
1772817737
transcode.$(OBJEXT): $(top_srcdir)/internal/compilers.h
1772917738
transcode.$(OBJEXT): $(top_srcdir)/internal/encoding.h
1773017739
transcode.$(OBJEXT): $(top_srcdir)/internal/gc.h
17740+
transcode.$(OBJEXT): $(top_srcdir)/internal/imemo.h
1773117741
transcode.$(OBJEXT): $(top_srcdir)/internal/inits.h
17742+
transcode.$(OBJEXT): $(top_srcdir)/internal/namespace.h
1773217743
transcode.$(OBJEXT): $(top_srcdir)/internal/object.h
17744+
transcode.$(OBJEXT): $(top_srcdir)/internal/sanitizers.h
1773317745
transcode.$(OBJEXT): $(top_srcdir)/internal/serial.h
17746+
transcode.$(OBJEXT): $(top_srcdir)/internal/set_table.h
1773417747
transcode.$(OBJEXT): $(top_srcdir)/internal/static_assert.h
1773517748
transcode.$(OBJEXT): $(top_srcdir)/internal/string.h
1773617749
transcode.$(OBJEXT): $(top_srcdir)/internal/transcode.h
1773717750
transcode.$(OBJEXT): $(top_srcdir)/internal/variable.h
17751+
transcode.$(OBJEXT): $(top_srcdir)/internal/vm.h
1773817752
transcode.$(OBJEXT): $(top_srcdir)/internal/warnings.h
1773917753
transcode.$(OBJEXT): {$(VPATH)}assert.h
17754+
transcode.$(OBJEXT): {$(VPATH)}atomic.h
1774017755
transcode.$(OBJEXT): {$(VPATH)}backward/2/assume.h
1774117756
transcode.$(OBJEXT): {$(VPATH)}backward/2/attributes.h
1774217757
transcode.$(OBJEXT): {$(VPATH)}backward/2/bool.h
@@ -17905,15 +17920,24 @@ transcode.$(OBJEXT): {$(VPATH)}internal/value_type.h
1790517920
transcode.$(OBJEXT): {$(VPATH)}internal/variable.h
1790617921
transcode.$(OBJEXT): {$(VPATH)}internal/warning_push.h
1790717922
transcode.$(OBJEXT): {$(VPATH)}internal/xmalloc.h
17923+
transcode.$(OBJEXT): {$(VPATH)}method.h
1790817924
transcode.$(OBJEXT): {$(VPATH)}missing.h
17925+
transcode.$(OBJEXT): {$(VPATH)}node.h
1790917926
transcode.$(OBJEXT): {$(VPATH)}onigmo.h
1791017927
transcode.$(OBJEXT): {$(VPATH)}oniguruma.h
17928+
transcode.$(OBJEXT): {$(VPATH)}ruby_assert.h
17929+
transcode.$(OBJEXT): {$(VPATH)}ruby_atomic.h
17930+
transcode.$(OBJEXT): {$(VPATH)}rubyparser.h
1791117931
transcode.$(OBJEXT): {$(VPATH)}shape.h
1791217932
transcode.$(OBJEXT): {$(VPATH)}st.h
1791317933
transcode.$(OBJEXT): {$(VPATH)}subst.h
17934+
transcode.$(OBJEXT): {$(VPATH)}thread_$(THREAD_MODEL).h
17935+
transcode.$(OBJEXT): {$(VPATH)}thread_native.h
1791417936
transcode.$(OBJEXT): {$(VPATH)}transcode.c
1791517937
transcode.$(OBJEXT): {$(VPATH)}transcode_data.h
17938+
transcode.$(OBJEXT): {$(VPATH)}vm_core.h
1791617939
transcode.$(OBJEXT): {$(VPATH)}vm_debug.h
17940+
transcode.$(OBJEXT): {$(VPATH)}vm_opts.h
1791717941
transcode.$(OBJEXT): {$(VPATH)}vm_sync.h
1791817942
util.$(OBJEXT): $(hdrdir)/ruby/ruby.h
1791917943
util.$(OBJEXT): $(top_srcdir)/internal/array.h

‎encoding.c‎

Lines changed: 58 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@
2828
#include "ruby/encoding.h"
2929
#include "ruby/util.h"
3030
#include "ruby_assert.h"
31+
#include "vm_core.h"
3132
#include "vm_sync.h"
3233
#include "ruby_atomic.h"
3334

@@ -421,6 +422,7 @@ int
421422
rb_enc_register(const char *name, rb_encoding *encoding)
422423
{
423424
int index;
425+
bool set_const = false;
424426

425427
GLOBAL_ENC_TABLE_LOCKING(enc_table) {
426428
index = enc_registered(enc_table, name);
@@ -439,9 +441,12 @@ rb_enc_register(const char *name, rb_encoding *encoding)
439441
}
440442
else {
441443
index = enc_register(enc_table, name, encoding);
442-
set_encoding_const(name, rb_enc_from_index(index));
444+
set_const = true;
443445
}
444446
}
447+
if (set_const) {
448+
set_encoding_const(name, rb_enc_from_index(index));
449+
}
445450
return index;
446451
}
447452

@@ -472,13 +477,14 @@ rb_enc_registered(const char *name)
472477
void
473478
rb_encdb_declare(const char *name)
474479
{
480+
int idx;
475481
GLOBAL_ENC_TABLE_LOCKING(enc_table) {
476-
int idx = enc_registered(enc_table, name);
482+
idx = enc_registered(enc_table, name);
477483
if (idx < 0) {
478484
idx = enc_register(enc_table, name, 0);
479485
}
480-
set_encoding_const(name, rb_enc_from_index(idx));
481486
}
487+
set_encoding_const(name, rb_enc_from_index(idx));
482488
}
483489

484490
static void
@@ -537,7 +543,6 @@ enc_replicate(struct enc_table *enc_table, const char *name, rb_encoding *encodi
537543
idx = enc_register(enc_table, name, encoding);
538544
if (idx < 0) rb_raise(rb_eArgError, "invalid encoding name: %s", name);
539545
set_base_encoding(enc_table, idx, encoding);
540-
set_encoding_const(name, rb_enc_from_index(idx));
541546
return idx;
542547
}
543548

@@ -552,7 +557,6 @@ enc_replicate_with_index(struct enc_table *enc_table, const char *name, rb_encod
552557
}
553558
if (idx >= 0) {
554559
set_base_encoding(enc_table, idx, origenc);
555-
set_encoding_const(name, rb_enc_from_index(idx));
556560
}
557561
else {
558562
rb_raise(rb_eArgError, "failed to replicate encoding");
@@ -575,6 +579,8 @@ rb_encdb_replicate(const char *name, const char *orig)
575579
r = enc_replicate_with_index(enc_table, name, rb_enc_from_index(origidx), idx);
576580
}
577581

582+
set_encoding_const(name, rb_enc_from_index(r));
583+
578584
return r;
579585
}
580586

@@ -588,6 +594,7 @@ rb_define_dummy_encoding(const char *name)
588594
rb_encoding *enc = enc_table->list[index].enc;
589595
ENC_SET_DUMMY((rb_raw_encoding *)enc);
590596
}
597+
set_encoding_const(name, rb_enc_from_index(index));
591598

592599
return index;
593600
}
@@ -605,6 +612,8 @@ rb_encdb_dummy(const char *name)
605612
ENC_SET_DUMMY((rb_raw_encoding *)enc);
606613
}
607614

615+
set_encoding_const(name, rb_enc_from_index(index));
616+
608617
return index;
609618
}
610619

@@ -671,18 +680,20 @@ enc_alias_internal(struct enc_table *enc_table, const char *alias, int idx)
671680
}
672681

673682
static int
674-
enc_alias(struct enc_table *enc_table, const char *alias, int idx)
683+
enc_alias(struct enc_table *enc_table, const char *alias, int idx, bool *set_const)
675684
{
676685
if (!valid_encoding_name_p(alias)) return -1;
677-
if (!enc_alias_internal(enc_table, alias, idx))
678-
set_encoding_const(alias, enc_from_index(enc_table, idx));
686+
if (!enc_alias_internal(enc_table, alias, idx)) {
687+
*set_const = true;
688+
}
679689
return idx;
680690
}
681691

682692
int
683693
rb_enc_alias(const char *alias, const char *orig)
684694
{
685695
int idx, r;
696+
bool set_const = false;
686697
GLOBAL_ENC_TABLE_LOCKING(enc_table) {
687698
enc_check_addable(enc_table, alias); // can raise
688699
}
@@ -691,7 +702,11 @@ rb_enc_alias(const char *alias, const char *orig)
691702
if (idx < 0) return -1;
692703

693704
GLOBAL_ENC_TABLE_LOCKING(enc_table) {
694-
r = enc_alias(enc_table, alias, idx);
705+
r = enc_alias(enc_table, alias, idx, &set_const);
706+
}
707+
708+
if (set_const) {
709+
set_encoding_const(alias, rb_enc_from_index(idx));
695710
}
696711

697712
return r;
@@ -701,14 +716,18 @@ int
701716
rb_encdb_alias(const char *alias, const char *orig)
702717
{
703718
int r;
719+
bool set_const = false;
704720

705721
GLOBAL_ENC_TABLE_LOCKING(enc_table) {
706722
int idx = enc_registered(enc_table, orig);
707723

708724
if (idx < 0) {
709725
idx = enc_register(enc_table, orig, 0);
710726
}
711-
r = enc_alias(enc_table, alias, idx);
727+
r = enc_alias(enc_table, alias, idx, &set_const);
728+
}
729+
if (set_const) {
730+
set_encoding_const(alias, rb_enc_from_index(r));
712731
}
713732

714733
return r;
@@ -717,34 +736,35 @@ rb_encdb_alias(const char *alias, const char *orig)
717736
static void
718737
rb_enc_init(struct enc_table *enc_table)
719738
{
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-
}
739+
RB_VM_LOCKING() {
740+
enc_table_expand(enc_table, ENCODING_COUNT + 1);
741+
if (!enc_table->names) {
742+
enc_table->names = st_init_strcasetable_with_size(ENCODING_LIST_CAPA);
743+
}
725744
#define OnigEncodingASCII_8BIT OnigEncodingASCII
726745
#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;
746+
ENC_REGISTER(ASCII_8BIT);
747+
ENC_REGISTER(UTF_8);
748+
ENC_REGISTER(US_ASCII);
749+
global_enc_ascii = enc_table->list[ENCINDEX_ASCII_8BIT].enc;
750+
global_enc_utf_8 = enc_table->list[ENCINDEX_UTF_8].enc;
751+
global_enc_us_ascii = enc_table->list[ENCINDEX_US_ASCII].enc;
733752
#undef ENC_REGISTER
734753
#undef OnigEncodingASCII_8BIT
735754
#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);
755+
ENCDB_REGISTER("UTF-16BE", UTF_16BE);
756+
ENCDB_REGISTER("UTF-16LE", UTF_16LE);
757+
ENCDB_REGISTER("UTF-32BE", UTF_32BE);
758+
ENCDB_REGISTER("UTF-32LE", UTF_32LE);
759+
ENCDB_REGISTER("UTF-16", UTF_16);
760+
ENCDB_REGISTER("UTF-32", UTF_32);
761+
ENCDB_REGISTER("UTF8-MAC", UTF8_MAC);
762+
763+
ENCDB_REGISTER("EUC-JP", EUC_JP);
764+
ENCDB_REGISTER("Windows-31J", Windows_31J);
746765
#undef ENCDB_REGISTER
747-
enc_table->count = ENCINDEX_BUILTIN_MAX;
766+
enc_table->count = ENCINDEX_BUILTIN_MAX;
767+
}
748768
}
749769

750770
rb_encoding *
@@ -865,7 +885,11 @@ int
865885
rb_enc_find_index(const char *name)
866886
{
867887
int i;
868-
ASSERT_vm_unlocking(); // it needs to be unlocked so it can call `load_encoding` if necessary
888+
#if RUBY_DEBUG > 0
889+
if (rb_multi_ractor_p() || !rb_enc_registered(name)) {
890+
ASSERT_vm_unlocking(); // it needs to be unlocked so it can call `load_encoding` if necessary
891+
}
892+
#endif
869893
GLOBAL_ENC_TABLE_LOCKING(enc_table) {
870894
i = enc_registered(enc_table, name);
871895
}
@@ -1812,6 +1836,7 @@ set_default_internal(VALUE klass, VALUE encoding)
18121836
static void
18131837
set_encoding_const(const char *name, rb_encoding *enc)
18141838
{
1839+
ASSERT_vm_unlocking();
18151840
VALUE encoding = rb_enc_from_encoding(enc);
18161841
char *s = (char *)name;
18171842
int haslower = 0, hasupper = 0, valid = 0;

‎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: 13 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -10360,7 +10360,12 @@ static void
1036010360
argf_lineno_setter(VALUE val, ID id, VALUE *var)
1036110361
{
1036210362
VALUE argf = *var;
10363-
int n = NUM2INT(val);
10363+
int n;
10364+
RB_VM_UNLOCK();
10365+
{
10366+
n = NUM2INT(val); // can context switch
10367+
}
10368+
RB_VM_LOCK();
1036410369
ARGF.last_lineno = ARGF.lineno = n;
1036510370
}
1036610371

@@ -14432,7 +14437,13 @@ argf_filename(VALUE argf)
1443214437
static VALUE
1443314438
argf_filename_getter(ID id, VALUE *var)
1443414439
{
14435-
return argf_filename(*var);
14440+
VALUE filename = Qnil;
14441+
RB_VM_UNLOCK();
14442+
{
14443+
filename = argf_filename(*var);
14444+
}
14445+
RB_VM_LOCK();
14446+
return filename;
1443614447
}
1443714448

1443814449
/*

0 commit comments

Comments
 (0)