Prevent to initialize MonitorMixin twice
Some libraries or tests unfortunately do something like:
require 'monitor' class Foo include MonitorMixin def initialize(*args) super() mon_initialize end end
This unfortunately ends up initializing the monitor twice.
If the two monitor initializations are done concurrently,
we can end up with two threads entering the same Monitor concurrently,
as they can acquire two different Mutex instances.
I'd proposed to raise an exception if
@mon_mutex is already set, like
raise "already initialized" if @mon_mutex.
This doesn't fully solve the problem if allocate is used and #initialize is called concurrently, but then I think it's the user's fault.
I originally found this problem while running Rails tests, but I can't find the exact source file anymore causing this.
Still, I think it's better to check here.
Updated by shugo (Shugo Maeda) over 1 year ago
- Assignee changed from shugo (Shugo Maeda) to Eregon (Benoit Daloze)
- Status changed from Closed to Assigned
Reverted r65822 because mon_initialize_spec.rb fails:
1) MonitorMixin#mon_initialize can be called in initialize_copy to get a new M utex and used with synchronize ERROR ThreadError: already initialized /home/shugo/src/ruby/lib/monitor.rb:255:in `mon_initialize' /home/shugo/src/ruby/spec/ruby/library/monitor/mon_initialize_spec.rb:19:in `initialize_copy' /home/shugo/src/ruby/spec/ruby/library/monitor/mon_initialize_spec.rb:28:in `initialize_dup' /home/shugo/src/ruby/spec/ruby/library/monitor/mon_initialize_spec.rb:28:in `dup' /home/shugo/src/ruby/spec/ruby/library/monitor/mon_initialize_spec.rb:28:in `block (2 levels) in <top (required)>' /home/shugo/src/ruby/spec/ruby/library/monitor/mon_initialize_spec.rb:4:in `<top (required)>'
mon_initialized may be used to re-initialize copied objects intentionally.
It's possible to add a new option such as
force: to mon_initialize, but
the default value should be
true for backward compatibility, and it's
not helpful to avoid unintentional re-initialization.
What do you think, Eregon?
Updated by znz (Kazuhiro NISHIYAMA) over 1 year ago
How about checking owner object?
diff --git a/lib/monitor.rb b/lib/monitor.rb index 288ed755ea..1a17e9bc7d 100644 --- a/lib/monitor.rb +++ b/lib/monitor.rb @@ -251,9 +251,13 @@ def initialize(*args) # Initializes the MonitorMixin after being included in a class or when an # object has been extended with the MonitorMixin def mon_initialize + if defined?(@mon_mutex) && @mon_mutex_owner_object_id == object_id + raise ThreadError, "already initialized" + end + @mon_mutex = Thread::Mutex.new + @mon_mutex_owner_object_id = object_id @mon_owner = nil @mon_count = 0 - @mon_mutex = Thread::Mutex.new end def mon_check_owner
Updated by Eregon (Benoit Daloze) over 1 year ago
Yes, it's non-trivial to fix unfortunately.
I remember doing the same in TruffleRuby and saw many Rails tests fail IIRC.
Also, we got a bug report with a variant to avoid re-initialization:
znz (Kazuhiro NISHIYAMA) Seems good and I think that is worth a try, does it pass test-all and test-spec?