Bug #12999: there still exist race conditions in require

• Status: Closed
• Assignee: -
• Target version: -
• Backport: 2.1: UNKNOWN, 2.2: UNKNOWN, 2.3: UNKNOWN

[shugo (Shugo Maeda)]

TestRequire#test_require_with_loaded_features_pop fails by increasing the number of threads,
so it seems that there still exist race conditions in require.

lexington:ruby$ cat t/require_test.rb
require "tempfile"

Tempfile.create(%w'bug-7530- .rb') {|script|
  script.close
  PATH = script.path
  THREADS = 10
  ITERATIONS_PER_THREAD = 1000

  THREADS.times.map {
    Thread.new do
      ITERATIONS_PER_THREAD.times do
        require PATH
        $".delete_if {|p| Regexp.new(PATH) =~ p}
      end
    end
  }.each(&:join)
  p :ok
}
lexington:ruby$ ./ruby t/require_test.rb
/home/shugo/local/lib/ruby/2.4.0/rubygems/core_ext/kernel_require.rb:55:in `require': destroyed thread shield - 0x00557a99ff5a78 (ThreadError)
	from /home/shugo/local/lib/ruby/2.4.0/rubygems/core_ext/kernel_require.rb:55:in `require'
	from t/require_test.rb:12:in `block (4 levels) in <main>'
	from t/require_test.rb:11:in `times'
	from t/require_test.rb:11:in `block (3 levels) in <main>'
lexington:ruby$ ./ruby t/require_test.rb
/home/shugo/local/lib/ruby/2.4.0/rubygems/core_ext/kernel_require.rb:55:in `require': Attempt to unlock a mutex which is locked by another thread (ThreadError)
	from /home/shugo/local/lib/ruby/2.4.0/rubygems/core_ext/kernel_require.rb:55:in `require'
	from t/require_test.rb:12:in `block (4 levels) in <main>'
	from t/require_test.rb:11:in `times'
	from t/require_test.rb:11:in `block (3 levels) in <main>'

[shugo (Shugo Maeda)]

• Related to Bug #7530: Concurrent loads fail with mutex errors added

[shugo (Shugo Maeda)]

In r56965, load_lock() was changed as follows:

    switch (rb_thread_shield_wait((VALUE)data)) {
      case Qfalse:
        data = (st_data_t)ftptr;
        st_insert(loading_tbl, data, (st_data_t)rb_thread_shield_new());
        return 0;
      case Qnil:
        return 0;
    }

However, if the thread shield is destroyed, it's not necessary to insert
a new entry to the loading table, is it?

With the following fix, ThreadError doesn't seem to occur:

diff --git a/load.c b/load.c
index 3cf9ce0..c37f8bc 100644
--- a/load.c
+++ b/load.c
@@ -745,9 +745,6 @@ load_lock(const char *ftptr)
     }
     switch (rb_thread_shield_wait((VALUE)data)) {
       case Qfalse:
-	data = (st_data_t)ftptr;
-	st_insert(loading_tbl, data, (st_data_t)rb_thread_shield_new());
-	return 0;
       case Qnil:
 	return 0;
     }
@@ -759,7 +756,10 @@ release_thread_shield(st_data_t *key, st_data_t *value, st_data_t done, int exis
 {
     VALUE thread_shield = (VALUE)*value;
     if (!existing) return ST_STOP;
-    if (done ? rb_thread_shield_destroy(thread_shield) : rb_thread_shield_release(thread_shield)) {
+    if (done) {
+	rb_thread_shield_destroy(thread_shield);
+    }
+    else if (rb_thread_shield_release(thread_shield)) {
 	/* still in-use */
 	return ST_CONTINUE;
     }

[shugo (Shugo Maeda)]

• Status changed from Open to Closed

Applied in changeset r56985.

---

Don't insert an entry to loading_tbl if another thread succeed to load.

If rb_thread_shield_wait() returns Qfalse, the file has been successfully
loaded by another thread, so there is no need to insert a new entry into
loading_tbl. [ruby-core:78464] [Bug #12999]