Project

General

Profile

Actions

Feature #15000

closed

Prevent to initialize MonitorMixin twice

Added by Eregon (Benoit Daloze) over 5 years ago. Updated over 5 years ago.

Status:
Closed
Target version:
-
[ruby-core:88504]

Description

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.

Actions #1

Updated by shugo (Shugo Maeda) over 5 years ago

  • 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]

Updated by shugo (Shugo Maeda) over 5 years ago

  • 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?

Updated by znz (Kazuhiro NISHIYAMA) over 5 years 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 5 years 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:
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?

Updated by shugo (Shugo Maeda) over 5 years ago

  • 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?

Actions #6

Updated by znz (Kazuhiro NISHIYAMA) over 5 years ago

  • 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]

Actions

Also available in: Atom PDF

Like0
Like0Like0Like0Like0Like0Like0