From 26b3f0b6a9489860e1d312369d784495b080daa3 Mon Sep 17 00:00:00 2001 From: nagachika Date: Tue, 21 Mar 2023 15:32:44 +0900 Subject: merge revision(s) 8ce2fb9bbbaea14737c84385b1573f743a30f773,3a0f6ce1d31eefd8af01b50f3632a64d64e8f8c1: [Backport #19415] 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. --- internal/thread.h | 1 + load.c | 3 ++- spec/ruby/core/kernel/shared/require.rb | 11 +++++++++++ spec/ruby/fixtures/code/concurrent_require_fixture.rb | 4 ++++ test/ruby/test_require.rb | 3 --- thread.c | 11 +++++++++++ 6 files changed, 29 insertions(+), 4 deletions(-) create mode 100644 spec/ruby/fixtures/code/concurrent_require_fixture.rb Use Thread.pass until thread.stop? to wait for thread to block [Bug #19415] It should be more reliable --- spec/ruby/fixtures/code/concurrent_require_fixture.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) --- internal/thread.h | 1 + load.c | 2 +- spec/ruby/core/kernel/shared/require.rb | 11 +++++++++++ spec/ruby/fixtures/code/concurrent_require_fixture.rb | 4 ++++ test/ruby/test_require.rb | 3 --- thread.c | 11 +++++++++++ version.h | 2 +- 7 files changed, 29 insertions(+), 5 deletions(-) create mode 100644 spec/ruby/fixtures/code/concurrent_require_fixture.rb diff --git a/internal/thread.h b/internal/thread.h index 919ad96580..c7fe16eb6b 100644 --- a/internal/thread.h +++ b/internal/thread.h @@ -28,6 +28,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); diff --git a/load.c b/load.c index bc0571124d..498c66a3fe 100644 --- a/load.c +++ b/load.c @@ -821,7 +821,7 @@ load_lock(rb_vm_t *vm, const char *ftptr, bool warn) (*init)(); return (char *)""; } - 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); diff --git a/spec/ruby/core/kernel/shared/require.rb b/spec/ruby/core/kernel/shared/require.rb index 9a2c38be1a..f4645b0242 100644 --- a/spec/ruby/core/kernel/shared/require.rb +++ b/spec/ruby/core/kernel/shared/require.rb @@ -237,6 +237,17 @@ describe :kernel_require, shared: true do }.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 diff --git a/spec/ruby/fixtures/code/concurrent_require_fixture.rb b/spec/ruby/fixtures/code/concurrent_require_fixture.rb new file mode 100644 index 0000000000..d4ce734183 --- /dev/null +++ b/spec/ruby/fixtures/code/concurrent_require_fixture.rb @@ -0,0 +1,4 @@ +object = ScratchPad.recorded +thread = Thread.new { object.require(__FILE__) } +Thread.pass until thread.stop? +ScratchPad.record(thread) diff --git a/test/ruby/test_require.rb b/test/ruby/test_require.rb index 77cdae64d9..f53bd2ce5f 100644 --- a/test/ruby/test_require.rb +++ b/test/ruby/test_require.rb @@ -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([:pre, :post], scratch, bug5754) - - assert_match(/circular require/, output) - assert_match(/in #{__method__}'$/o, output) } ensure $VERBOSE = verbose diff --git a/thread.c b/thread.c index de29340713..b1b5dd0f53 100644 --- a/thread.c +++ b/thread.c @@ -5040,6 +5040,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. * diff --git a/version.h b/version.h index b5bbcf46d5..d72eff87f2 100644 --- a/version.h +++ b/version.h @@ -11,7 +11,7 @@ # define RUBY_VERSION_MINOR RUBY_API_VERSION_MINOR #define RUBY_VERSION_TEENY 4 #define RUBY_RELEASE_DATE RUBY_RELEASE_YEAR_STR"-"RUBY_RELEASE_MONTH_STR"-"RUBY_RELEASE_DAY_STR -#define RUBY_PATCHLEVEL 204 +#define RUBY_PATCHLEVEL 205 #define RUBY_RELEASE_YEAR 2023 #define RUBY_RELEASE_MONTH 3 -- cgit v1.2.3