Bug #17359
Ractor copy mode is not Ractor-safe
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) about 2 months ago
mmm, should not call clone
here?
Updated by marcandre (Marc-Andre Lafortune) about 2 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) about 2 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) about 2 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) about 2 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) about 2 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) about 2 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) about 2 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 formove
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) about 2 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) about 2 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))
Updated by marcandre (Marc-Andre Lafortune) about 2 months ago
ko1 (Koichi Sasada) wrote in #note-10:
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) about 2 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) about 2 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) about 2 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) about 2 months ago
Dan0042 (Daniel DeLorme) wrote in #note-14:
I think it's feasible.
initialize_clone
andinitialize_copy
are mostly (only?) used becauseclone
anddup
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.