Feature #15000: Prevent to initialize MonitorMixin twice

• Status: Closed
• Assignee: znz (Kazuhiro NISHIYAMA)
• Target version: -

[Eregon (Benoit Daloze)]

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.

[shugo (Shugo Maeda)]

• Status changed from Open to Closed

Applied in changeset trunk|r65822.

---

lib/monitor.rb: prevent to initialize MonitorMixin twice

Suggested by Benoit Daloze. [ruby-core:88504] [Feature #15000]

[shugo (Shugo Maeda)]

• Status changed from Closed to Assigned
• Assignee changed from shugo (Shugo Maeda) to Eregon (Benoit Daloze)

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?

[znz (Kazuhiro NISHIYAMA)]

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

[Eregon (Benoit Daloze)]

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:
https://github.com/oracle/truffleruby/issues/1428

@znz (Kazuhiro NISHIYAMA) Seems good and I think that is worth a try, does it pass test-all and test-spec?

[shugo (Shugo Maeda)]

• Assignee changed from Eregon (Benoit Daloze) to znz (Kazuhiro NISHIYAMA)

znz (Kazuhiro NISHIYAMA) wrote:

How about checking owner object?

Could you commit it if it passes tests?

[znz (Kazuhiro NISHIYAMA)]

• Status changed from Assigned to Closed

Applied in changeset trunk|r66215.

---

lib/monitor.rb: prevent to initialize MonitorMixin twice

and allow to initialize again when obj.dup.

Suggested by Benoit Daloze. [ruby-core:88504] [Feature #15000]