Skip to content

Commit 8ce2fb9

Browse files
casperisfinebyroot
authored andcommitted
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.
1 parent 3ab3455 commit 8ce2fb9

File tree

6 files changed

+29
-4
lines changed

6 files changed

+29
-4
lines changed

internal/thread.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@ VALUE rb_get_coverages(void);
2929
int rb_get_coverage_mode(void);
3030
VALUE rb_default_coverage(int);
3131
VALUE rb_thread_shield_new(void);
32+
bool rb_thread_shield_owned(VALUE self);
3233
VALUE rb_thread_shield_wait(VALUE self);
3334
VALUE rb_thread_shield_release(VALUE self);
3435
VALUE rb_thread_shield_destroy(VALUE self);

load.c

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -850,7 +850,8 @@ load_lock(rb_vm_t *vm, const char *ftptr, bool warn)
850850
st_insert(loading_tbl, (st_data_t)ftptr, data);
851851
return (char *)ftptr;
852852
}
853-
if (warn) {
853+
854+
if (warn && rb_thread_shield_owned((VALUE)data)) {
854855
VALUE warning = rb_warning_string("loading in progress, circular require considered harmful - %s", ftptr);
855856
rb_backtrace_each(rb_str_append, warning);
856857
rb_warning("%"PRIsVALUE, warning);

spec/ruby/core/kernel/shared/require.rb

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -237,6 +237,17 @@
237237
}.should complain(/circular require considered harmful/, verbose: true)
238238
ScratchPad.recorded.should == [:loaded]
239239
end
240+
241+
ruby_bug "#17340", ''...'3.3' do
242+
it "loads a file concurrently" do
243+
path = File.expand_path "concurrent_require_fixture.rb", CODE_LOADING_DIR
244+
ScratchPad.record(@object)
245+
-> {
246+
@object.require(path)
247+
}.should_not complain(/circular require considered harmful/, verbose: true)
248+
ScratchPad.recorded.join
249+
end
250+
end
240251
end
241252

242253
describe "(non-extensioned path)" do
Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
object = ScratchPad.recorded
2+
thread = Thread.new { object.require(__FILE__) }
3+
thread.wakeup unless thread.stop?
4+
ScratchPad.record(thread)

test/ruby/test_require.rb

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -562,9 +562,6 @@ class << (output = "")
562562

563563
assert_equal(true, (t1_res ^ t2_res), bug5754 + " t1:#{t1_res} t2:#{t2_res}")
564564
assert_equal([:pre, :post], scratch, bug5754)
565-
566-
assert_match(/circular require/, output)
567-
assert_match(/in #{__method__}'$/o, output)
568565
}
569566
ensure
570567
$VERBOSE = verbose

thread.c

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4920,6 +4920,17 @@ rb_thread_shield_new(void)
49204920
return thread_shield;
49214921
}
49224922

4923+
bool
4924+
rb_thread_shield_owned(VALUE self)
4925+
{
4926+
VALUE mutex = GetThreadShieldPtr(self);
4927+
if (!mutex) return false;
4928+
4929+
rb_mutex_t *m = mutex_ptr(mutex);
4930+
4931+
return m->fiber == GET_EC()->fiber_ptr;
4932+
}
4933+
49234934
/*
49244935
* Wait a thread shield.
49254936
*

0 commit comments

Comments
 (0)