Bug #18243: Ractor.make_shareable does not freeze the receiver of a Proc but allows accessing ivars of it

• Status: Closed
• Assignee: -
• Target version: -
• Backport: 2.6: UNKNOWN, 2.7: UNKNOWN, 3.0: UNKNOWN

[Eregon (Benoit Daloze)]

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

[Eregon (Benoit Daloze)]

• Related to Bug #18232: Ractor.make_shareable is broken in code loaded with RubyVM::InstructionSequence.load_from_binary added

[Eregon (Benoit Daloze)]

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).

[Eregon (Benoit Daloze)]

Relevant related discussion: https://bugs.ruby-lang.org/issues/18137#note-2

[Eregon (Benoit Daloze)]

• Description updated (diff)

Sorry, I missed to paste some part of the snippet, fixed now.

[Eregon (Benoit Daloze)]

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.

[Eregon (Benoit Daloze)]

• Related to Feature #18276: `Proc#bind_call(obj)` same as `obj.instance_exec(..., &proc_obj)` added

[Dan0042 (Daniel DeLorme)]

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.

[Eregon (Benoit Daloze)]

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.

[Eregon (Benoit Daloze)]

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.

[ko1 (Koichi Sasada)]

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

[ko1 (Koichi Sasada)]

• 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]

[shan (Shannon Skipper)]

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

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.

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

[Eregon (Benoit Daloze)]

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.