Project

General

Profile

Actions

Bug #22291

open

Instance variables should be forbidden on Ractor-shareable objects

Bug #22291: Instance variables should be forbidden on Ractor-shareable objects

Added by jhawthorn (John Hawthorn) 5 days ago. Updated 1 day ago.

Status:
Open
Assignee:
-
Target version:
-
[ruby-core:126569]

Description

Usually Ractor-shareable objects are frozen, but we have a number of objects which are shareable even though they aren't frozen (and likely will have more in the future from C extensions).

  • Classes/Modules (ignored in this issue, they have special ivar handling)
  • Ractor
  • Ractor::Port
  • Isolated procs/lambdas from Ractor.shareable_proc
  • ENV

These are set shareable without the usual checks that all referenced objects are also shareable. This causes a potential issue, because these objects could have unshareable instance variables, breaking the Ractor invariant (potentially causing race conditions and segmentation faults).

Currently, we attempt to avoid this by having a check on ivars read that forbids reading these from a non-main Ractor

R = Ractor.new {}
R.instance_variable_set(:@iv, +"mutable")

Ractor.new { R.instance_variable_get(:@iv) }.value
#=> Ractor::IsolationError: can not access instance variables of shareable objects from non-main Ractors

This is detected by checking on ivar read for objects which are shareable but not frozen.

However this isn't sound because most of these unshareable objects can be frozen, turning the check off.

R = Ractor.new {}
R.instance_variable_set(:@iv, +"mutable")
R.freeze

Ractor.new { R.instance_variable_get(:@iv) }.value
# => no error, created Ractor got access to an object it shouldn't

(weird quirk: ENV#freeze raises, but you can Kernel.instance_method(:freeze).bind_call(ENV) so it's still an issue)

I propose that we fix this by forbidding instance variable writes to objects which are shareable (whether or not thy are frozen). This removes the need to check on read and maintains the Ractor invariant.

R = Ractor.new {}
R.instance_variable_set(:@iv, 123)
#=> Ractor::IsolationError: can not set instance variables of shareable Ractor objects

Ractor.new {
  R.instance_variable_get(:@foo) # => always nil, because ivars are forbidden
}

These objects behave essentially as though they're frozen, but only the IVs are frozen.

This is an important issue to solve both for correctness, and because I want us to decide the semantics so that they can be implemented in ZJIT. Currently ZJIT won't compile ivar reads on multi-ractor mode because of this issue.

(I will link a patch implementing this shortly)

Updated by Eregon (Benoit Daloze) 4 days ago Actions #1 [ruby-core:126575]

jhawthorn (John Hawthorn) wrote:

I propose that we fix this by forbidding instance variable writes to objects which are frozen.

... to objects which are shareable, right?
(it's already forbidden on frozen objects)

I think this makes sense but it makes me wonder, should making these objects shareable also freeze them? (e.g. for Ractor, Ractor::Port, Ractor.shareable_proc, ENV)
That would simplify the model.
Classes/Modules are already special so that would be the exception we probably need to keep.

IIRC @ko1 (Koichi Sasada) once said that Ractor instances being shareable yet not frozen was intentional to allow ivars on them.

Updated by rkh (Konstantin Haase) 3 days ago Actions #2 [ruby-core:126581]

This only really affects Ractor and ENV. It is impossible to set instance variables on Ractor::Port, even in an initializer in a subclass, and they come out frozen. Similarly, Ractor.shareable_proc produces frozen objects.

Modules, as you said, have special handling.

So… my proposal would be:

  • ENV can only be modified by the main ractor, similar to modules created on that ractor.
  • Ractor.new returns frozen instances. Any instance variables set during initialization will be made shareable. This still allows Ractor subclasses to make use of instance variables.

Updated by jhawthorn (John Hawthorn) 1 day ago · Edited Actions #3 [ruby-core:126588]

  • Description updated (diff)

Thanks for the correction, edited the description.

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

IIRC @ko1 (Koichi Sasada) once said that Ractor instances being shareable yet not frozen was intentional to allow ivars on them.

I discussed this with @ko1 (Koichi Sasada) and this didn't come up. I don't think it ever makes sense currently to set instance variables on a Ractor (which currently can only be done by the main Ractor).

I'm not proposing any changes to Classes/Modules (#22226 is an accepted proposal to make a change there, this issue should not affect or be affected by that)


Both comments above suggest freezing some or all of these objects. That would solve the invariant violation, but I think there's a question of what "freezing" is supposed to represent. Whether it's just that instance variables are immutable, or if the object's specific state is immutable as well.

Looking at some specifics:

  • Ractor.shareable_proc is already frozen, that seems fine to me
  • Ractor is a lot like Thread. You can freeze a Thread, which freezes thread and fiber local variables. (execution still runs, it's a bit of a weird thing to do)
  • Ractor::Port is a lot like Queue. Queue as of #17146 (from @Eregon (Benoit Daloze)) can't be frozen, which makes Ractor::Port being frozen by default seems strange. Seems like it was a workaround in b9188901c07649c3af3a5f925ec0dead444a4134, but we'd have to ask @ko1 (Koichi Sasada)
  • ENV forbids .freeze, introduced in #15920, however it can be frozen via bind_call (see #17738). This is acceptable for avoiding user confusion, but would not be sufficient for Ractor's safety guarantees.

When we make this decision we also need to consider other Ractor-safe objects and data structures that a C extension might define. We haven't yet published a proper API for this, but it would be weird for ex. a "Ractor safe hash" to be frozen.

IMO our past decisions suggest that frozen means that a Ractor-safe object's state should also be frozen, and not just the ivars, and so we should just forbid ivars on those objects.

Actions

Also available in: PDF Atom