Bug #5754

Double require bug in 1.9.3

Added by Evan Phoenix over 3 years ago. Updated over 3 years ago.

[ruby-core:41618]
Status:Closed
Priority:Normal
Assignee:Nobuyoshi Nakada
ruby -v:- Backport:

Description

There appears a bug in the handling of concurrent requires of the same file. If a thread (t2) is waiting for another thread (t1) to finish requiring the file, but requiring the file raises an exception, t1 signals t2 and t2 tries to require the file itself. But due to how the loading table is managed, if t1 attempts to require the file again right away (or if another thread were to), it doesn't see an entry in the loading table even though t2 is currently loading the file. This causes t1 to begin requiring the file again or sometimes it raises a ThreadError instead. Here is the code I write for rubyspec to show the bug:

    start = false
    fin = false

    $scratch = []
    $con1_ready = false
    $con1_raise = true
    path = "path/to/content/below.rb"

    t1_res = nil
    t2_res = nil

    t1 = Thread.new do
      lambda {
        require(path)
      }.should raise_error(RuntimeError)

      # This hits the bug. Because MRI removes it's internal lock from a table
      # when the exception is raised, this #require doesn't see that t2 is
      # in the middle of requiring the file, so this #require runs when it should
      # not.
      #
      # Sometimes this raises a ThreadError also, but I'm not sure why.
      t1_res = require(path)

      Thread.pass until fin
      $scratch << :t1_post
    end

    t2 = Thread.new do
      Thread.pass until $con1_ready
      begin
        t2_res = require(path)
        $scratch << :t2_post
      ensure
        fin = true
      end
    end

    t1.join
    t2.join

    raise "bug" unless t1_res == false
    raise "bug" unless t2_res == true

    $scratch == [:con_pre, :con_pre, :con_post, :t2_post, :t1_post]

Here is the file being required:

$scratch << :con_pre
$con1_ready = true
sleep 0.5
if $con1_raise
$con1_raise = false
raise "con1"
end
sleep 0.5
$scratch << :con_post


Related issues

Related to Ruby trunk - Bug #5768: TestRequire#test_race_exceptionで競合するケースがまだある Closed 12/17/2011

Associated revisions

Revision 34027
Added by Nobuyoshi Nakada over 3 years ago

  • load.c (load_unlock): all threads requiring one file should share same loading barrier, so it must be kept alive while those are waiting on it. [Bug #5754]

Revision 34027
Added by Nobuyoshi Nakada over 3 years ago

  • load.c (load_unlock): all threads requiring one file should share same loading barrier, so it must be kept alive while those are waiting on it. [Bug #5754]

Revision 34039
Added by Nobuyoshi Nakada over 3 years ago

  • load.c (load_unlock): release loading barrier and then remove it from loading_table if it is not in-use. [Bug #5754]
  • thread.c (rb_barrier_release, rb_barrier_destroy): return whether any other threads are waiting on it.

Revision 34039
Added by Nobuyoshi Nakada over 3 years ago

  • load.c (load_unlock): release loading barrier and then remove it from loading_table if it is not in-use. [Bug #5754]
  • thread.c (rb_barrier_release, rb_barrier_destroy): return whether any other threads are waiting on it.

Revision 34046
Added by Nobuyoshi Nakada over 3 years ago

  • test/ruby/test_require.rb (test_race_exception): get rid of not-guaranteed timing issue. [Bug #5754]

Revision 34046
Added by Nobuyoshi Nakada over 3 years ago

  • test/ruby/test_require.rb (test_race_exception): get rid of not-guaranteed timing issue. [Bug #5754]

History

#1 Updated by Evan Phoenix over 3 years ago

I missed pruning out one bit from rubyspec so I've fixed it and put the code into a gist: https://gist.github.com/1469546

#2 Updated by Nobuyoshi Nakada over 3 years ago

  • Status changed from Open to Closed
  • % Done changed from 0 to 100

This issue was solved with changeset r34027.
Evan, thank you for reporting this issue.
Your contribution to Ruby is greatly appreciated.
May Ruby be with you.


  • load.c (load_unlock): all threads requiring one file should share same loading barrier, so it must be kept alive while those are waiting on it. [Bug #5754]

#3 Updated by Yui NARUSE over 3 years ago

  • Status changed from Closed to Assigned
  • Assignee set to Nobuyoshi Nakada

By r34027, test_race_excption(TestRequire) fails as following:

test_race_excption(TestRequire) [/extdisk/chkbuild/chkbuild/tmp/build/ruby-trunk/20111214T140500Z/ruby/test/ruby/test_require.rb:393]:
.
<[false, true]> expected but was
<[true, false]>.

http://www.rubyist.net/~akr/chkbuild/debian/ruby-trunk/log/20111214T140500Z.log.html.gz

#4 Updated by Nobuyoshi Nakada over 3 years ago

  • ruby -v changed from ruby 1.9.3p0 (2011-10-30 revision 33570) [x86_64-darwin11.2.0] to -

Hi,

(11/12/14 23:56), Yui NARUSE wrote:

By r34027, test_race_excption(TestRequire) fails as following:

test_race_excption(TestRequire) [/extdisk/chkbuild/chkbuild/tmp/build/ruby-trunk/20111214T140500Z/ruby/test/ruby/test_require.rb:393]:
.
<[false, true]> expected but was
<[true, false]>.

http://www.rubyist.net/~akr/chkbuild/debian/ruby-trunk/log/20111214T140500Z.log.html.gz

It sounds like a timing bug in the test code.

--
Nobu Nakada

#5 Updated by Yura Sokolov over 3 years ago

it doesn't see an entry in the loading table even though t2 is currently loading the file

Has it something common with #5727 ?

#6 Updated by Nobuyoshi Nakada over 3 years ago

  • Status changed from Assigned to Closed

This issue was solved with changeset r34039.
Evan, thank you for reporting this issue.
Your contribution to Ruby is greatly appreciated.
May Ruby be with you.


  • load.c (load_unlock): release loading barrier and then remove it from loading_table if it is not in-use. [Bug #5754]
  • thread.c (rb_barrier_release, rb_barrier_destroy): return whether any other threads are waiting on it.

Also available in: Atom PDF