Bug #17359: Ractor copy mode is not Ractor-safe

• Status: Open
• Priority: Normal
• Assignee: ko1 (Koichi Sasada)
• Target version: -
• ruby -v: ruby 3.0.0dev (2020-11-30T10:06:25Z master 89774a938a) [x86_64-darwin18]
• Backport: 2.5: UNKNOWN, 2.6: UNKNOWN, 2.7: UNKNOWN

[marcandre (Marc-Andre Lafortune)]

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?

[ko1 (Koichi Sasada)]

mmm, should not call clone here?

[marcandre (Marc-Andre Lafortune)]

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.

[Dan0042 (Daniel DeLorme)]

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.

[ko1 (Koichi Sasada)]

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.

[ko1 (Koichi Sasada)]

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.

[ko1 (Koichi Sasada)]

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

[Eregon (Benoit Daloze)]

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.

[ko1 (Koichi Sasada)]

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?

[marcandre (Marc-Andre Lafortune)]

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.

[ko1 (Koichi Sasada)]

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 ?

[marcandre (Marc-Andre Lafortune)]

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.

[Dan0042 (Daniel DeLorme)]

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.

[marcandre (Marc-Andre Lafortune)]

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.

[Dan0042 (Daniel DeLorme)]

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

[marcandre (Marc-Andre Lafortune)]

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.