Project

General

Profile

Bug #14742

Deadlock when autoloading different constants in the same file from multiple threads

Added by eugeneius (Eugene Kenny) about 1 year ago. Updated about 1 year ago.

Status:
Closed
Priority:
Normal
Assignee:
-
Target version:
-
ruby -v:
ruby 2.6.0dev (2018-05-08 trunk 63355) [x86_64-darwin17]
[ruby-core:86935]

Description

The following example deadlocks consistently:

# a.rb
autoload :Foo, __dir__ + "/b"
autoload :Bar, __dir__ + "/b"

t1 = Thread.new { Foo }
t2 = Thread.new { Bar }

t1.join
t2.join

# b.rb
module Foo; end
module Bar; end
$ ruby a.rb
Traceback (most recent call last):
    1: from a.rb:7:in `<main>'
a.rb:7:in `join': No live threads left. Deadlock? (fatal)
3 threads, 3 sleeps current:0x00007ffa817ae680 main thread:0x00007ffa8140bf30
* #<Thread:0x00007ffa818797f8 sleep_forever>
   rb_thread_t:0x00007ffa8140bf30 native:0x00007fff9b3fa380 int:1
   a.rb:7:in `join'
   a.rb:7:in `<main>'
* #<Thread:0x00007ffa830379a8@a.rb:4 sleep_forever>
   rb_thread_t:0x00007ffa817ace60 native:0x0000700007e7b000 int:0 mutex:0x00007ffa817ae680 cond:1
    depended by: tb_thread_id:0x00007ffa8140bf30
   /Users/eugene/.rbenv/versions/2.6.0-dev/lib/ruby/2.6.0/rubygems/core_ext/kernel_require.rb:59:in `require'
   /Users/eugene/.rbenv/versions/2.6.0-dev/lib/ruby/2.6.0/rubygems/core_ext/kernel_require.rb:59:in `require'
   a.rb:4:in `block in <main>'
* #<Thread:0x00007ffa830372c8@a.rb:5 sleep_forever>
   rb_thread_t:0x00007ffa817ae680 native:0x0000700007f7e000 int:0
   /Users/eugene/src/autoload_thread_safety_test/b.rb:1:in `<top (required)>'
   /Users/eugene/.rbenv/versions/2.6.0-dev/lib/ruby/2.6.0/rubygems/core_ext/kernel_require.rb:59:in `require'
   /Users/eugene/.rbenv/versions/2.6.0-dev/lib/ruby/2.6.0/rubygems/core_ext/kernel_require.rb:59:in `require'
   a.rb:5:in `block in <main>'

Associated revisions

Revision b7413113
Added by normal about 1 year ago

variable.c: fix multiple autoload with identical file

We need to ensure autoload declarations pointing to the same
feature (aka "file") can wait on each other to avoid deadlock
situations.

So, reorganize autoload data structures to maintain a
feature => autoload_data_i mapping, and have module constant
tables point to the new autoload_const struct instead of
directly to autoload_data_i. This allows multiple
autoload_const structs to refer to the SAME autoload_data_i
struct, and with it, the on-stack autoload_state.waitq.

The end result is different constants can share the same waitq
(tied to the feature name), and not deadlock each other during
loading.

Thanks to Eugene Kenny for the bug report and reproducible test case.

Reported-by: Eugene Kenny elkenny@gmail.com

  • variable.c (autoload_featuremap): new global (struct autoload_const): new per-const struct (struct autoload_state): reference autoload_const instead of autoload_data_i (struct autoload_data_i): remove per-const (autoload_i_mark): delete from autoload_featuremap if unreferenced (autoload_c_mark): new dmark callback (autoload_c_free): new dfree callback (autoload_c_memsize): new memsize callback (autoload_const_type): new data type (get_autoload_data): set autoload_const as well (rb_autoload_str): use new data structures (autoload_delete): cleanup from autoload_featuremap (check_autoload_required): adjust for new internals (rb_autoloading_value): ditto (struct autoload_const_set_args): remove, redundant with autoload_const (const_tbl_update): adjust for new internals (autoload_const_set): ditto (autoload_require): ditto (autoload_reset): ditto (rb_autoload_load): ditto (rb_const_set): ditto (current_autoload_data): ditto (set_const_visibility): ditto
  • test/ruby/test_autoload.rb (test_autoload_same_file): new test [ruby-core:86935] [Bug #14742]

git-svn-id: svn+ssh://ci.ruby-lang.org/ruby/trunk@63387 b2dd03c8-39d4-4d8f-98ff-823fe69b080e

Revision 63387
Added by normalperson (Eric Wong) about 1 year ago

variable.c: fix multiple autoload with identical file

We need to ensure autoload declarations pointing to the same
feature (aka "file") can wait on each other to avoid deadlock
situations.

So, reorganize autoload data structures to maintain a
feature => autoload_data_i mapping, and have module constant
tables point to the new autoload_const struct instead of
directly to autoload_data_i. This allows multiple
autoload_const structs to refer to the SAME autoload_data_i
struct, and with it, the on-stack autoload_state.waitq.

The end result is different constants can share the same waitq
(tied to the feature name), and not deadlock each other during
loading.

Thanks to Eugene Kenny for the bug report and reproducible test case.

Reported-by: Eugene Kenny elkenny@gmail.com

  • variable.c (autoload_featuremap): new global (struct autoload_const): new per-const struct (struct autoload_state): reference autoload_const instead of autoload_data_i (struct autoload_data_i): remove per-const (autoload_i_mark): delete from autoload_featuremap if unreferenced (autoload_c_mark): new dmark callback (autoload_c_free): new dfree callback (autoload_c_memsize): new memsize callback (autoload_const_type): new data type (get_autoload_data): set autoload_const as well (rb_autoload_str): use new data structures (autoload_delete): cleanup from autoload_featuremap (check_autoload_required): adjust for new internals (rb_autoloading_value): ditto (struct autoload_const_set_args): remove, redundant with autoload_const (const_tbl_update): adjust for new internals (autoload_const_set): ditto (autoload_require): ditto (autoload_reset): ditto (rb_autoload_load): ditto (rb_const_set): ditto (current_autoload_data): ditto (set_const_visibility): ditto
  • test/ruby/test_autoload.rb (test_autoload_same_file): new test [ruby-core:86935] [Bug #14742]

Revision 63387
Added by normal about 1 year ago

variable.c: fix multiple autoload with identical file

We need to ensure autoload declarations pointing to the same
feature (aka "file") can wait on each other to avoid deadlock
situations.

So, reorganize autoload data structures to maintain a
feature => autoload_data_i mapping, and have module constant
tables point to the new autoload_const struct instead of
directly to autoload_data_i. This allows multiple
autoload_const structs to refer to the SAME autoload_data_i
struct, and with it, the on-stack autoload_state.waitq.

The end result is different constants can share the same waitq
(tied to the feature name), and not deadlock each other during
loading.

Thanks to Eugene Kenny for the bug report and reproducible test case.

Reported-by: Eugene Kenny elkenny@gmail.com

  • variable.c (autoload_featuremap): new global (struct autoload_const): new per-const struct (struct autoload_state): reference autoload_const instead of autoload_data_i (struct autoload_data_i): remove per-const (autoload_i_mark): delete from autoload_featuremap if unreferenced (autoload_c_mark): new dmark callback (autoload_c_free): new dfree callback (autoload_c_memsize): new memsize callback (autoload_const_type): new data type (get_autoload_data): set autoload_const as well (rb_autoload_str): use new data structures (autoload_delete): cleanup from autoload_featuremap (check_autoload_required): adjust for new internals (rb_autoloading_value): ditto (struct autoload_const_set_args): remove, redundant with autoload_const (const_tbl_update): adjust for new internals (autoload_const_set): ditto (autoload_require): ditto (autoload_reset): ditto (rb_autoload_load): ditto (rb_const_set): ditto (current_autoload_data): ditto (set_const_visibility): ditto
  • test/ruby/test_autoload.rb (test_autoload_same_file): new test [ruby-core:86935] [Bug #14742]

Revision 6726038d
Added by normal about 1 year ago

variable.c: fix autoload object lifetimes and leak

We must not call normal Hash methods inside GC free callback,
either, however identity hash may be used.

[ruby-core:86935] [Bug #14742]

git-svn-id: svn+ssh://ci.ruby-lang.org/ruby/trunk@63389 b2dd03c8-39d4-4d8f-98ff-823fe69b080e

Revision 63389
Added by normalperson (Eric Wong) about 1 year ago

variable.c: fix autoload object lifetimes and leak

We must not call normal Hash methods inside GC free callback,
either, however identity hash may be used.

[ruby-core:86935] [Bug #14742]

Revision 63389
Added by normal about 1 year ago

variable.c: fix autoload object lifetimes and leak

We must not call normal Hash methods inside GC free callback,
either, however identity hash may be used.

[ruby-core:86935] [Bug #14742]

Revision ec959fbb
Added by normal about 1 year ago

variable.c: fix multiple autoload with identical file (again)

We need to ensure autoload declarations pointing to the same
feature (aka "file") can wait on each other to avoid deadlock
situations.

So, reorganize autoload data structures to maintain a
feature => autoload_data_i mapping, and have module constant
tables point to the new autoload_const struct instead of
directly to autoload_data_i. This allows multiple
autoload_const structs to refer to the SAME autoload_data_i
struct, and with it, the on-stack autoload_state.waitq.

The end result is different constants can share the same waitq
(tied to the feature name), and not deadlock each other during
loading.

Thanks to Eugene Kenny for the bug report and reproducible test case.

Reported-by: Eugene Kenny elkenny@gmail.com

  • variable.c (autoload_featuremap): new global (struct autoload_const): new per-const struct (struct autoload_state): reference autoload_const instead of autoload_data_i (struct autoload_data_i): remove per-const (autoload_i_mark): delete from autoload_featuremap if unreferenced (autoload_c_mark): new dmark callback (autoload_c_free): new dfree callback (autoload_c_memsize): new memsize callback (autoload_const_type): new data type (get_autoload_data): set autoload_const as well (rb_autoload_str): use new data structures (autoload_delete): cleanup from autoload_featuremap (check_autoload_required): adjust for new internals (rb_autoloading_value): ditto (struct autoload_const_set_args): remove, redundant with autoload_const (const_tbl_update): adjust for new internals (autoload_const_set): ditto (autoload_require): ditto (autoload_reset): ditto (rb_autoload_load): ditto (rb_const_set): ditto (current_autoload_data): ditto (set_const_visibility): ditto
  • test/ruby/test_autoload.rb (test_autoload_same_file): new test (test_no_leak): new test [ruby-core:86935] [Bug #14742]

git-svn-id: svn+ssh://ci.ruby-lang.org/ruby/trunk@63392 b2dd03c8-39d4-4d8f-98ff-823fe69b080e

Revision 63392
Added by normalperson (Eric Wong) about 1 year ago

variable.c: fix multiple autoload with identical file (again)

We need to ensure autoload declarations pointing to the same
feature (aka "file") can wait on each other to avoid deadlock
situations.

So, reorganize autoload data structures to maintain a
feature => autoload_data_i mapping, and have module constant
tables point to the new autoload_const struct instead of
directly to autoload_data_i. This allows multiple
autoload_const structs to refer to the SAME autoload_data_i
struct, and with it, the on-stack autoload_state.waitq.

The end result is different constants can share the same waitq
(tied to the feature name), and not deadlock each other during
loading.

Thanks to Eugene Kenny for the bug report and reproducible test case.

Reported-by: Eugene Kenny elkenny@gmail.com

  • variable.c (autoload_featuremap): new global (struct autoload_const): new per-const struct (struct autoload_state): reference autoload_const instead of autoload_data_i (struct autoload_data_i): remove per-const (autoload_i_mark): delete from autoload_featuremap if unreferenced (autoload_c_mark): new dmark callback (autoload_c_free): new dfree callback (autoload_c_memsize): new memsize callback (autoload_const_type): new data type (get_autoload_data): set autoload_const as well (rb_autoload_str): use new data structures (autoload_delete): cleanup from autoload_featuremap (check_autoload_required): adjust for new internals (rb_autoloading_value): ditto (struct autoload_const_set_args): remove, redundant with autoload_const (const_tbl_update): adjust for new internals (autoload_const_set): ditto (autoload_require): ditto (autoload_reset): ditto (rb_autoload_load): ditto (rb_const_set): ditto (current_autoload_data): ditto (set_const_visibility): ditto
  • test/ruby/test_autoload.rb (test_autoload_same_file): new test (test_no_leak): new test [ruby-core:86935] [Bug #14742]

Revision 63392
Added by normal about 1 year ago

variable.c: fix multiple autoload with identical file (again)

We need to ensure autoload declarations pointing to the same
feature (aka "file") can wait on each other to avoid deadlock
situations.

So, reorganize autoload data structures to maintain a
feature => autoload_data_i mapping, and have module constant
tables point to the new autoload_const struct instead of
directly to autoload_data_i. This allows multiple
autoload_const structs to refer to the SAME autoload_data_i
struct, and with it, the on-stack autoload_state.waitq.

The end result is different constants can share the same waitq
(tied to the feature name), and not deadlock each other during
loading.

Thanks to Eugene Kenny for the bug report and reproducible test case.

Reported-by: Eugene Kenny elkenny@gmail.com

  • variable.c (autoload_featuremap): new global (struct autoload_const): new per-const struct (struct autoload_state): reference autoload_const instead of autoload_data_i (struct autoload_data_i): remove per-const (autoload_i_mark): delete from autoload_featuremap if unreferenced (autoload_c_mark): new dmark callback (autoload_c_free): new dfree callback (autoload_c_memsize): new memsize callback (autoload_const_type): new data type (get_autoload_data): set autoload_const as well (rb_autoload_str): use new data structures (autoload_delete): cleanup from autoload_featuremap (check_autoload_required): adjust for new internals (rb_autoloading_value): ditto (struct autoload_const_set_args): remove, redundant with autoload_const (const_tbl_update): adjust for new internals (autoload_const_set): ditto (autoload_require): ditto (autoload_reset): ditto (rb_autoload_load): ditto (rb_const_set): ditto (current_autoload_data): ditto (set_const_visibility): ditto
  • test/ruby/test_autoload.rb (test_autoload_same_file): new test (test_no_leak): new test [ruby-core:86935] [Bug #14742]

History

Updated by normalperson (Eric Wong) about 1 year ago

Yes, this is an old bug and not caused by my changes, I guess.
It requires reworking some internal data structures in
variable.c to provide feature mapping to autoload_data_i
so different autoloads referring to the same file (aka "feature")
can safely wait on each other.

Will work on this tomorrow, I need sleep :<

#3

Updated by normalperson (Eric Wong) about 1 year ago

  • Status changed from Open to Closed

Applied in changeset trunk|r63387.


variable.c: fix multiple autoload with identical file

We need to ensure autoload declarations pointing to the same
feature (aka "file") can wait on each other to avoid deadlock
situations.

So, reorganize autoload data structures to maintain a
feature => autoload_data_i mapping, and have module constant
tables point to the new autoload_const struct instead of
directly to autoload_data_i. This allows multiple
autoload_const structs to refer to the SAME autoload_data_i
struct, and with it, the on-stack autoload_state.waitq.

The end result is different constants can share the same waitq
(tied to the feature name), and not deadlock each other during
loading.

Thanks to Eugene Kenny for the bug report and reproducible test case.

Reported-by: Eugene Kenny elkenny@gmail.com

  • variable.c (autoload_featuremap): new global (struct autoload_const): new per-const struct (struct autoload_state): reference autoload_const instead of autoload_data_i (struct autoload_data_i): remove per-const (autoload_i_mark): delete from autoload_featuremap if unreferenced (autoload_c_mark): new dmark callback (autoload_c_free): new dfree callback (autoload_c_memsize): new memsize callback (autoload_const_type): new data type (get_autoload_data): set autoload_const as well (rb_autoload_str): use new data structures (autoload_delete): cleanup from autoload_featuremap (check_autoload_required): adjust for new internals (rb_autoloading_value): ditto (struct autoload_const_set_args): remove, redundant with autoload_const (const_tbl_update): adjust for new internals (autoload_const_set): ditto (autoload_require): ditto (autoload_reset): ditto (rb_autoload_load): ditto (rb_const_set): ditto (current_autoload_data): ditto (set_const_visibility): ditto
  • test/ruby/test_autoload.rb (test_autoload_same_file): new test [ruby-core:86935] [Bug #14742]

Updated by normalperson (Eric Wong) about 1 year ago

Sorry for the delay, r63387 should fix it.

Updated by normalperson (Eric Wong) about 1 year ago

Eric Wong normalperson@yhbt.net wrote:

Sorry for the delay, r63387 should fix it.

Reverted for now. Some problems need fixing but I can't reproduce
on my 32-bit system (and my 64-bit machines have problems).

Updated by eugeneius (Eugene Kenny) about 1 year ago

It looks like the fix was un-reverted in r63392. Thank you for working on this, Eric!

Also available in: Atom PDF