Skip to content

Commit

Permalink
Only emit circular dependency warning for owned thread shields
Browse files Browse the repository at this point in the history
[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.
  • Loading branch information
casperisfine authored and byroot committed Feb 8, 2023
1 parent 3ab3455 commit 8ce2fb9
Show file tree
Hide file tree
Showing 6 changed files with 29 additions and 4 deletions.
1 change: 1 addition & 0 deletions internal/thread.h
Expand Up @@ -29,6 +29,7 @@ VALUE rb_get_coverages(void);
int rb_get_coverage_mode(void);
VALUE rb_default_coverage(int);
VALUE rb_thread_shield_new(void);
bool rb_thread_shield_owned(VALUE self);
VALUE rb_thread_shield_wait(VALUE self);
VALUE rb_thread_shield_release(VALUE self);
VALUE rb_thread_shield_destroy(VALUE self);
Expand Down
3 changes: 2 additions & 1 deletion load.c
Expand Up @@ -850,7 +850,8 @@ load_lock(rb_vm_t *vm, const char *ftptr, bool warn)
st_insert(loading_tbl, (st_data_t)ftptr, data);
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);
rb_backtrace_each(rb_str_append, warning);
rb_warning("%"PRIsVALUE, warning);
Expand Down
11 changes: 11 additions & 0 deletions spec/ruby/core/kernel/shared/require.rb
Expand Up @@ -237,6 +237,17 @@
}.should complain(/circular require considered harmful/, verbose: true)
ScratchPad.recorded.should == [:loaded]
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)
ScratchPad.recorded.join
end
end
end

describe "(non-extensioned path)" do
Expand Down
4 changes: 4 additions & 0 deletions spec/ruby/fixtures/code/concurrent_require_fixture.rb
@@ -0,0 +1,4 @@
object = ScratchPad.recorded
thread = Thread.new { object.require(__FILE__) }
thread.wakeup unless thread.stop?
ScratchPad.record(thread)
3 changes: 0 additions & 3 deletions test/ruby/test_require.rb
Expand Up @@ -562,9 +562,6 @@ class << (output = "")

assert_equal(true, (t1_res ^ t2_res), bug5754 + " t1:#{t1_res} t2:#{t2_res}")
assert_equal([:pre, :post], scratch, bug5754)

assert_match(/circular require/, output)
assert_match(/in #{__method__}'$/o, output)
}
ensure
$VERBOSE = verbose
Expand Down
11 changes: 11 additions & 0 deletions thread.c
Expand Up @@ -4920,6 +4920,17 @@ rb_thread_shield_new(void)
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.
*
Expand Down

0 comments on commit 8ce2fb9

Please sign in to comment.