Project

General

Profile

Actions

Bug #17359

open

Ractor copy mode is not Ractor-safe

Added by marcandre (Marc-Andre Lafortune) 4 months ago. Updated 4 months ago.

Status:
Open
Priority:
Normal
Target version:
-
ruby -v:
ruby 3.0.0dev (2020-11-30T10:06:25Z master 89774a938a) [x86_64-darwin18]
[ruby-core:101174]

Description

It should not be possible to mutate an object across Ractors, but the copy mode allows it currently:

class Foo
  attr_accessor :x

  def initialize_copy(*)
    $last = self
    super
  end
end
o = Foo.new
o.x = 42

r = Ractor.new(o) do |copy|
  puts copy.x # => 42
  Ractor.yield :sync
  Ractor.yield :sync
  puts copy.x # => 666
end
r.take # => :sync
$last.x = 666
r.take # => :sync
r.take

Maybe the copy object should be marked as moved?

Updated by ko1 (Koichi Sasada) 4 months ago

mmm, should not call clone here?

Updated by marcandre (Marc-Andre Lafortune) 4 months ago

ko1 (Koichi Sasada) wrote in #note-1:

mmm, should not call clone here?

Sorry, I don't understand. Above code should be same with initialize_copy => initialize_clone, if that's what you mean?

The issue I'm pointing out is that one can register the new copy in the initial Ractor and use that reference later to mutate the object.

Updated by Dan0042 (Daniel DeLorme) 4 months ago

The initialize_copy is executed in the main ractor, that's why it can set a global variable. It should be executed in the sub-ractor I think.

Updated by ko1 (Koichi Sasada) 4 months ago

Dan0042 (Daniel DeLorme) wrote in #note-3:

The initialize_copy is executed in the main ractor, that's why it can set a global variable. It should be executed in the sub-ractor I think.

It is not acceptable because the src object is accessed from main and sub (initialize_copy) simultaneously.

Updated by ko1 (Koichi Sasada) 4 months ago

marcandre (Marc-Andre Lafortune) wrote in #note-2:

The issue I'm pointing out is that one can register the new copy in the initial Ractor and use that reference later to mutate the object.

I couldn't guess that the src ractor can access copied object with initialize_copy/clone.

Updated by ko1 (Koichi Sasada) 4 months ago

One idea is prohibit initialize_copy written in Ruby, for ractor_copy.
But I'm not sure how it is feasible.

ko1@aluminium:~$ gem-codesearch 'def initialize_clone' | wc -l
120
ko1@aluminium:~$ gem-codesearch 'def initialize_copy' | wc -l
3430

Updated by Eregon (Benoit Daloze) 4 months ago

An idea: copy everything in the source Ractor as currently, and then move the resulting copied object.
That way, the source Ractor might keep references to copied objects, but it won't be able to use them.
This actually means a partial copy for move on top of the original deep copy though.

Updated by ko1 (Koichi Sasada) 4 months ago

Eregon (Benoit Daloze) wrote in #note-7:

An idea: copy everything in the source Ractor as currently, and then move the resulting copied object.
That way, the source Ractor might keep references to copied objects, but it won't be able to use them.
This actually means a partial copy for move on top of the original deep copy though.

Complete idea, but it makes two objects per one. mmm.

Doing it if user-defined initialize_copy/clone is detected?

Updated by marcandre (Marc-Andre Lafortune) 4 months ago

I feel it is important not to break the contract of initialize_copy (or otherwise replace it with another one, but that doesn't solve the problem).

Nobody commented on my idea: make the deep copy as currently (in the current Ractor), then move it to the new Ractor (like send(new_copy, move: true))

Ractor.new(obj) { |copy| ... }
# equivalent to
copy = deep_clone(obj)
r = Ractor.new { copy = Ractor.receive ... }
r.send(copy, move: true)

My example code above would (correctly) raise an exception when calling attempting to mutate the copy from the wrong Ractor.

Updated by ko1 (Koichi Sasada) 4 months ago

Nobody commented on my idea: make the deep copy as currently (in the current Ractor), then move it to the new Ractor (like send(new_copy, move: true))

same as https://bugs.ruby-lang.org/issues/17359#note-7 ?

Updated by marcandre (Marc-Andre Lafortune) 4 months ago

ko1 (Koichi Sasada) wrote in #note-10:

same as https://bugs.ruby-lang.org/issues/17359#note-7 ?

Ohh, sorry 😅 I got confused because I didn't understand how moving worked. Now I understand better. So yes, there would be a performance penalty involved 😢. And yes, only if user-defined initialize_copy/clone is detected.

Updated by Dan0042 (Daniel DeLorme) 4 months ago

I was under the impression that ractor-copy worked via dump+load sort of like marshaling, in fact I thought that was the basis for #17298 basket communication APIs. Is that not (no longer?) the case? Because Marshal.load doesn't call initialize_copy so it seems like the same rules should apply here.

Updated by marcandre (Marc-Andre Lafortune) 4 months ago

THis was changed to actual traversal and cloning. This way any intermediate object that is shareable is simply shared. Other objects were also not marshalable but can be cloned.

Updated by Dan0042 (Daniel DeLorme) 4 months ago

ko1 (Koichi Sasada) wrote in #note-6:

One idea is prohibit initialize_copy written in Ruby, for ractor_copy.
But I'm not sure how it is feasible.

ko1@aluminium:~$ gem-codesearch 'def initialize_clone' | wc -l
120
ko1@aluminium:~$ gem-codesearch 'def initialize_copy' | wc -l
3430

I think it's feasible. initialize_clone and initialize_copy are mostly (only?) used because clone and dup only perform shallow copies. See the examples below. Since ractor-copy already performs a deep-copy, the extra copy/cloning in these examples is redundant. Actually... it's buggy? Since the initialize_clone is executed in the sending ractor, the objects are created as part of the sending ractor's heap rather than the receiving ractor's heap?

activemodel-6.0.0/lib/active_model/attribute_set.rb
72:    def initialize_clone(_)
73-      @attributes = attributes.clone
74-      super
75-    end

regexp_parser-1.6.0/lib/regexp_parser/expression/classes/group.rb
36:      def initialize_clone(orig)
37-        @name = orig.name.dup
38-        super
39-      end
40-    end

regexp_parser-1.6.0/lib/regexp_parser/expression/quantifier.rb
15:    def initialize_clone(orig)
16-      @text = orig.text.dup
17-      super
18-    end

regexp_parser-1.6.0/lib/regexp_parser/expression/subexpression.rb
15:    def initialize_clone(orig)
16-      self.expressions = orig.expressions.map(&:clone)
17-      super
18-    end

regexp_parser-1.6.0/lib/regexp_parser/expression.rb
24:    def initialize_clone(orig)
25-      self.text       = (orig.text       ? orig.text.dup         : nil)
26-      self.options    = (orig.options    ? orig.options.dup      : nil)
27-      self.quantifier = (orig.quantifier ? orig.quantifier.clone : nil)
28-      super
29-    end

actionpack-6.0.0/lib/action_controller/metal/strong_parameters.rb
1017:      def initialize_copy(source)
1018-        super
1019-        @parameters = @parameters.dup
1020-      end

actionpack-6.0.0/lib/action_dispatch/http/content_security_policy.rb
157:    def initialize_copy(other)
158-      @directives = other.directives.deep_dup
159-    end

actionpack-6.0.0/lib/action_dispatch/middleware/flash.rb
147:      def initialize_copy(other)
148-        if other.now_is_loaded?
149-          @now = other.now.dup
150-          @now.flash = self
151-        end
152-        super
153-      end

actionpack-6.0.0/lib/action_dispatch/middleware/stack.rb
95:    def initialize_copy(other)
96-      self.middlewares = other.middlewares.dup
97-    end

actionview-6.0.0/lib/action_view/path_set.rb
22:    def initialize_copy(other)
23-      @paths = other.paths.dup
24-      self
25-    end

Updated by marcandre (Marc-Andre Lafortune) 4 months ago

Dan0042 (Daniel DeLorme) wrote in #note-14:

I think it's feasible. initialize_clone and initialize_copy are mostly (only?) used because clone and dup only

Indeed, I imagine that this will be most of the cases, as of today (i.e. before Ractor).

My concern is to have some way for user classes to handle special cases of deep-cloning for Ractor, in the same way that calling freeze from Ractor.make_shareable provides a basic way to handle deep-freezing. Without one, and without Ractor::LVar, there would be no way to handle these special cases.

Or maybe we can begin without it and we can revisit as use-cases arise, I don't know.

Actions

Also available in: Atom PDF