Project

General

Profile

Feature #15000

Prevent to initialize MonitorMixin twice

Added by Eregon (Benoit Daloze) 7 months ago. Updated 3 months ago.

Status:
Closed
Priority:
Normal
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.

Associated revisions

Revision 8d68f422
Added by shugo (Shugo Maeda) 4 months ago

lib/monitor.rb: prevent to initialize MonitorMixin twice

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

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

Revision 65822
Added by shugo (Shugo Maeda) 4 months ago

lib/monitor.rb: prevent to initialize MonitorMixin twice

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

Revision 65822
Added by shugo (Shugo Maeda) 4 months ago

lib/monitor.rb: prevent to initialize MonitorMixin twice

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

Revision 1eba8ecb
Added by znz (Kazuhiro NISHIYAMA) 3 months ago

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]

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

Revision 66215
Added by znz (Kazuhiro NISHIYAMA) 3 months ago

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]

Revision 66215
Added by kazu 3 months ago

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]

History

#1

Updated by shugo (Shugo Maeda) 4 months 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) 4 months 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) 4 months 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) 4 months 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) 4 months 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?

#6

Updated by znz (Kazuhiro NISHIYAMA) 3 months 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]

Also available in: Atom PDF