Bug #18243
closedRactor.make_shareable does not freeze the receiver of a Proc but allows accessing ivars of it
Description
class C
attr_accessor :foo
def setter_proc
Ractor.make_shareable(-> v { @foo = v })
end
end
c = C.new
c.foo = 1
p c
proc = c.setter_proc
p c.frozen?
Ractor.new(proc) { |s| s.call(42) }.take
p c
gives
#<C:0x00007f2a3aae7a98 @foo=1>
false
#<C:0x00007f2a3aae7a98 @foo=42> # BUG
But that must be a bug, it means the non-main Ractor can directly mutate an object from the main Ractor.
I found this while thinking about https://github.com/ruby/ostruct/pull/29/files and
whether Ractor.make_shareable
would freeze @table
and the OpenStruct
instance (I think it needs to).
Repro code for ostruct and changing ostruct.rb to $setter = ::Ractor.make_shareable(setter_proc)
:
require 'ostruct'
os = OpenStruct.new
os.foo = 1
$setter.call(2)
p os
Ractor.new($setter) { |s| s.call(42) }.take
p os
gives
#<OpenStruct foo=2>
<internal:ractor>:267: warning: Ractor is experimental, and the behavior may change in future versions of Ruby! Also there are many implementation issues.
#<OpenStruct foo=42> # BUG
Updated by Eregon (Benoit Daloze) almost 3 years ago
- Related to Bug #18232: Ractor.make_shareable is broken in code loaded with RubyVM::InstructionSequence.load_from_binary added
Updated by Eregon (Benoit Daloze) almost 3 years ago
Smaller repro:
$ ruby -e 'Object.new.instance_eval { p object_id; proc = Ractor.make_shareable -> { self }; Ractor.new(proc) { |c| p c.call.object_id }.take }'
60
<internal:ractor>:267: warning: Ractor is experimental, and the behavior may change in future versions of Ruby! Also there are many implementation issues.
60
In contrast to:
$ ruby -e 'a=Object.new; Ractor.make_shareable -> { a }'
<internal:ractor>:816:in `make_shareable': can not make shareable Proc because it can refer unshareable object #<Object:0x00007fd8bdd8f688> from variable `a' (Ractor::IsolationError)
from -e:1:in `<main>'
It seems there is no check for self
but there should be, it's like a capture local variable.
Either it raises like for local vars or it Ractor.make_shareable
the self
.
If it raises ostruct.rb will break but that's fixable by Ractor.make_shareable self
first which is probably best for clarity anyway, cc @marcandre (Marc-Andre Lafortune).
Updated by Eregon (Benoit Daloze) almost 3 years ago
Relevant related discussion: https://bugs.ruby-lang.org/issues/18137#note-2
Updated by Eregon (Benoit Daloze) almost 3 years ago
- Description updated (diff)
Sorry, I missed to paste some part of the snippet, fixed now.
Updated by Eregon (Benoit Daloze) almost 3 years ago
After some discussion with @ko1 (Koichi Sasada), it seems there are different cases.
Sharing self
is the only way to preserve the Proc's context correctly, i.e., method calls, @ivar, etc still work in the Proc
I think raising if self
is not shareable might be a good way to let the user choose appropriately.
It's not clear if the user wants to make the Proc's self
shareable, or if they want to change the Proc's self to a shareable object.
Then user can choose:
someProc = -> { ... }; Ractor.make_shareable(self); Ractor.make_shareable(someProc)
-
someProc = -> { ... }; Ractor.make_shareable(someProc.with_self(nil))
.
define_method(:name) do; ...; end
is special case because the receiver will be changed when called. And that's expected for users too. So maybe some special integration between define_method
and Ractor makes sense (to skip the self
check since not needed in that case).
Current semantics of passing the Proc across Ractors while it refers to unshareable objects is violation of semantics, i.e., I don't think raising on Proc#call is good idea.
------------------------------------------------------------------------
Ractor.make_shareable(obj, copy: false) -> shareable_obj
------------------------------------------------------------------------
Make obj shareable between ractors.
obj and all the objects it refers to will be frozen, unless they are
already shareable.
I believe we need to preserve this simple definition to avoid explosive amounts of complexity and edge cases.
Updated by Eregon (Benoit Daloze) almost 3 years ago
- Related to Feature #18276: `Proc#bind_call(obj)` same as `obj.instance_exec(..., &proc_obj)` added
Updated by Dan0042 (Daniel DeLorme) almost 3 years ago
It's not uncommon for a proc to have behavior that is independent of self
. Let's say:
Ractor.make_shareable(-> x { x + 1 })
In this case it's irrelevant if self
is shareable or not, because it's never used in the proc. There's no method call or ivar access for self
. It would be inconvenient if that kind of proc required self
to be shareable. From the posts by @Eregon (Benoit Daloze) it was unclear to me if "self must be shareable" was intended only for an explicit reference to self
.
Updated by Eregon (Benoit Daloze) almost 3 years ago
Dan0042 (Daniel DeLorme) wrote in #note-7:
It's not uncommon for a proc to have behavior that is independent of
self
.
Yes, but it's not so trivial to assert it is the case for any non-trivial block/Proc.
Changing the self is breaking the context in which it was written, and I think that should be avoided in most cases.
define_method(name) { ... }
is clear that the self is not the surrounding self, but that's about the only core method with those semantics and it needs to be like that of course.
From the posts by @Eregon (Benoit Daloze) it was unclear to me if "self must be shareable" was intended only for an explicit reference to
self
.
All Proc capture self
, there is no other way currently.
I think we need define_method(name, make_shareable: true) { ... }
(name up for discussion) and there we know it does not matter what the surrounding self
is.
Ractor.make_shareable(some_proc)
must make some_proc
and all objects reachable from it shareable, hence the self
captured by the Proc must be shareable, or it breaks the entire concept of Ractor isolation and shareable.
So either it does Ractor.make_shareable(proc_self)
or it raises an exception.
Changing the proc's self
on make_shareable
seems way too confusing, I don't even consider it a reasonable possibility.
Updated by Eregon (Benoit Daloze) almost 3 years ago
From #18276,
I think the best solution by far is Ractor.make_shareable(proc.bind(nil))
(concise, clear, clean as it does not pass unshared object to other Ractor), and raise on Ractor.make_shareable(proc)
if the Proc's self
is not shareable.
And of course Ractor.make_shareable(self); Ractor.make_shareable(proc)
is also possible if desired, and explicit so safe and expected/intuitive.
Updated by ko1 (Koichi Sasada) over 2 years ago
At least we need fix it.
This PR simply prohibits make_shareable()
if the proc's self is unshareable.
https://github.com/ruby/ruby/pull/5232
Updated by ko1 (Koichi Sasada) over 2 years ago
- Status changed from Open to Closed
Applied in changeset git|cce331272b07636d536c8227288ab3fbcf24e2aa.
Ractor.make_shareable
checks proc's sefl
Ractor.make_shareable(proc_obj)
raises an IsolationError
if the self of proc_obj
is not a shareable object.
[Bug #18243]
Updated by shan (Shannon Skipper) over 2 years ago
Eregon (Benoit Daloze) wrote in #note-9:
From #18276,
I think the best solution by far isRactor.make_shareable(proc.bind(nil))
(concise, clear, clean as it does not pass unshared object to other Ractor), and raise onRactor.make_shareable(proc)
if the Proc'sself
is not shareable.
And of courseRactor.make_shareable(self); Ractor.make_shareable(proc)
is also possible if desired, and explicit so safe and expected/intuitive.
A proc.bind(nil)
seems like a nice solution. This gist, for example, was using Procs with unfrozen receivers and now raises an IsolationError
, but I can't think of a reasonable way to use nil.instance_eval
when the proc isn't created up front (updated the gist to use a thinly veiled instance_eval for demonstration purposes). https://gist.github.com/havenwood/6ac4d8c32f8af0364c27ffa26241db67
Updated by Eregon (Benoit Daloze) over 2 years ago
I think in general it's good the caller code is aware it needs a shareable self
, otherwise there might be big surprises.
For instance if your script adds
def self.helper_method
...
end
and used that within the block, it would not work since self
is nil
and not the main object (or in an instance method of some class it'd be nil
instead of the instance).
The Parallel do
there is a good hint there might be a change of receiver, without that it could be very confusing why the self
was magically changed to nil
/why the context around the block is not the expected one.