22 Jul
2023
22 Jul
'23
5:26 a.m.
Issue #19535 has been updated by nagachika (Tomoyuki Chikanaga).
Backport changed from 2.7: DONTNEED, 3.0: DONTNEED, 3.1: DONTNEED, 3.2: REQUIRED to 2.7:
DONTNEED, 3.0: DONTNEED, 3.1: DONTNEED, 3.2: DONE
ruby_3_2 fa72ba72f8c64fd0fa87c8f68cbc31f2e7b94b00 merged revision(s)
54dbd8bea8a79bfcdefa471c1717c6cd28022f33.
----------------------------------------
Bug #19535: Instance variables order is unpredictable on objects with
`OBJ_TOO_COMPLEX_SHAPE_ID`
https://bugs.ruby-lang.org/issues/19535#change-103953
* Author: byroot (Jean Boussier)
* Status: Closed
* Priority: Normal
* ruby -v: 3.2.1
* Backport: 2.7: DONTNEED, 3.0: DONTNEED, 3.1: DONTNEED, 3.2: DONE
----------------------------------------
### Context
I've been helping the Mastodon folks in investigating a weird Marshal deserialization
bug they randomly experience since they upgraded to Ruby 3.2:
https://github.com/mastodon/mastodon/issues/23644
Ultimately the bug comes from a circular dependency issues in the object graph that is
serialized when one call `Marshal.dump` on an `ActiveRecord::Base` object.
A simplified reproduction to better explain the problem is:
```ruby
class Status
def normal_order
@attributes = { id: 42 }
@relations = { self => 1 }
self
end
def inverse_order
@relations = nil
@attributes = { id: 42 }
@relations = { self => 1 }
self
end
def hash
@attributes.fetch(:id)
end
end
s = Marshal.load(Marshal.dump(Status.new.normal_order))
s = Marshal.load(Marshal.dump(Status.new.inverse_order))
```
In short, that `Status` object is both the top level object, and is referenced as a key in
a hash, in that same payload. It also defined a custom `#hash` method, that requires some
other attribute to be set.
It all "works" as long as `@attributes` is dumped before `@relations`.
### Problem
The above micro-reproduction uses two different shapes to demonstrate the ordering issues,
but in both case the ordering is predictable.
However if you generate too many shapes from a single class, it will be marked as
`TOO_COMPLEX` and future instance will have their instance variables backed by an
`id_table`, which is unordered, and will cause a similar issue.
I definitely consider this a bug on the Rails side, and I will do what I can so that Rails
doesn't depend on that implicit ordering.
However it's unlikely we'll be able to fix older version, and other users may run
into this issue when upgrading to Ruby 3.2, so I think it may be worth to try to preserve
some sort of predicable ordering, at least for a few more versions.
Additionally, debugging it was made particularly difficult, because it would work fine
initially, and then break after enough shapes had been generated. Generally speaking I
think such semi-predictable behavior is much worse than a fully random behavior (similar
to how Go randomize keys order in their maps).
### Historical behavior
On Ruby 3.1 and older, the instance variables ordering was defined by the order in which
each ivar appeared for the very first time:
```ruby
class Foo
def set
@a = 1
@b = 2
@c = 3
self
end
def inverse_order
@c = 3
@b = 2
@a = 1
self
end
end
p Foo.new.set.instance_variables # => [:@a, :@b, :@c]
p Foo.new.inverse_order.instance_variables # => [:@a, :@b, :@c]
```
This means that the order could be different from once execution of the program to
another, but would remain stable inside a single process.
On 3.2, it's now defined by the order in which each ivar appeared in that specific
object instance:
```ruby
[:@a, :@b, :@c]
[:@c, :@b, :@a]
```
Except, if the object is backed by an `id_table`, in which case it's fully
unpredictable.
### Possible changes
I discussed this with @tenderlovemaking, and he suggested we could change the `id_table`
for an `st_table` so that the ordering could be predictable again, and would behave like
objects with a non-complex shape.
Another possibility would be to preserve the observable behavior of 3.1 and older.
Or of course we could clearly specify that the ordering is random, but if so I think it
would be wise to make it always random so that this class of bugs has a much higher chance
to be caught early in testing rather than in production.
cc @Eregon as I presume this has implications on TruffleRuby as well.
--
https://bugs.ruby-lang.org/