Only emit circular dependency warning for owned thread shields

[Bug #19415]

If multiple threads attemps to load the same file concurrently
it's not a circular dependency issue.

So we check that the existing ThreadShield is owner by the current
fiber before warning about circular dependencies.
This commit is contained in:
Jean Boussier 2023-02-06 12:30:50 +01:00 коммит произвёл Jean Boussier
Родитель 28da990984
Коммит fa49651e05
6 изменённых файлов: 25 добавлений и 4 удалений

Просмотреть файл

@ -29,6 +29,7 @@ VALUE rb_get_coverages(void);
int rb_get_coverage_mode(void); int rb_get_coverage_mode(void);
VALUE rb_default_coverage(int); VALUE rb_default_coverage(int);
VALUE rb_thread_shield_new(void); VALUE rb_thread_shield_new(void);
bool rb_thread_shield_owned(VALUE self);
VALUE rb_thread_shield_wait(VALUE self); VALUE rb_thread_shield_wait(VALUE self);
VALUE rb_thread_shield_release(VALUE self); VALUE rb_thread_shield_release(VALUE self);
VALUE rb_thread_shield_destroy(VALUE self); VALUE rb_thread_shield_destroy(VALUE self);

3
load.c
Просмотреть файл

@ -850,7 +850,8 @@ load_lock(rb_vm_t *vm, const char *ftptr, bool warn)
st_insert(loading_tbl, (st_data_t)ftptr, data); st_insert(loading_tbl, (st_data_t)ftptr, data);
return (char *)ftptr; return (char *)ftptr;
} }
if (warn) {
if (warn && rb_thread_shield_owned((VALUE)data)) {
VALUE warning = rb_warning_string("loading in progress, circular require considered harmful - %s", ftptr); VALUE warning = rb_warning_string("loading in progress, circular require considered harmful - %s", ftptr);
rb_backtrace_each(rb_str_append, warning); rb_backtrace_each(rb_str_append, warning);
rb_warning("%"PRIsVALUE, warning); rb_warning("%"PRIsVALUE, warning);

Просмотреть файл

@ -237,6 +237,16 @@ describe :kernel_require, shared: true do
}.should complain(/circular require considered harmful/, verbose: true) }.should complain(/circular require considered harmful/, verbose: true)
ScratchPad.recorded.should == [:loaded] ScratchPad.recorded.should == [:loaded]
end end
ruby_bug "#17340", ''...'3.3' do
it "loads a file concurrently" do
path = File.expand_path "concurrent_require_fixture.rb", CODE_LOADING_DIR
ScratchPad.record(@object)
-> {
@object.require(path)
}.should_not complain(/circular require considered harmful/, verbose: true)
end
end
end end
describe "(non-extensioned path)" do describe "(non-extensioned path)" do

Просмотреть файл

@ -0,0 +1 @@
Thread.new { ScratchPad.recorded.require(__FILE__) }.join(0.1)

Просмотреть файл

@ -562,9 +562,6 @@ class TestRequire < Test::Unit::TestCase
assert_equal(true, (t1_res ^ t2_res), bug5754 + " t1:#{t1_res} t2:#{t2_res}") assert_equal(true, (t1_res ^ t2_res), bug5754 + " t1:#{t1_res} t2:#{t2_res}")
assert_equal([:pre, :post], scratch, bug5754) assert_equal([:pre, :post], scratch, bug5754)
assert_match(/circular require/, output)
assert_match(/in #{__method__}'$/o, output)
} }
ensure ensure
$VERBOSE = verbose $VERBOSE = verbose

Просмотреть файл

@ -4920,6 +4920,17 @@ rb_thread_shield_new(void)
return thread_shield; return thread_shield;
} }
bool
rb_thread_shield_owned(VALUE self)
{
VALUE mutex = GetThreadShieldPtr(self);
if (!mutex) return false;
rb_mutex_t *m = mutex_ptr(mutex);
return m->fiber == GET_EC()->fiber_ptr;
}
/* /*
* Wait a thread shield. * Wait a thread shield.
* *