Repository navigation
Show chain of references in Ractor errors - #15983
etiennebarrie wants to merge 3 commits into
Conversation
This comment has been minimized.
This comment has been minimized.
luke-gru
left a comment
There was a problem hiding this comment.
If chain is guaranteed to be non-nil when printing, I think an assertion that chain != Qnil would be helpful. Otherwise lgtm!
|
I think for code like this: str = "hi"
str.instance_variable_set("@b", proc { })
obj = Object.new
obj.instance_variable_set("@a", str)
ary = [obj]
Ractor.make_shareable(ary)It would be nice to get more type info like: The "on Type" part requires tracking the |
|
I was about to suggest including the type as well. |
ee0ecd3 to
eceb099
Compare
I find it's a bit too redundant, each type is listed twice, once on the instance itself, and then on the object that refers to that instance on the line below. Here's what I got now for that example: ../../test.rb:6:in 'Ractor.make_shareable': Proc's self is not shareable: #<Proc:0x00000001034c20b0 ../../test.rb:2> (Ractor::IsolationError)
from block's self (an instance of Object)
from instance variable @b of an instance of String
from instance variable @a of an instance of Object
from Array element at index 0
from ../../test.rb:6:in '<main>'"main" is now "an instance of Object" which is not great but in a lot of cases, proc's self will be of another class, the one where the proc was defined. I also changed "from struct member :foo" to "from member :foo of an instance of S" because The updated result for the example at the top: test.rb:12:in 'Ractor.make_shareable': Proc's self is not shareable: #<Proc:0x000000010893d860 test.rb:10 (lambda)> (Ractor::IsolationError)
from block's self (an instance of Object)
from instance variable @e of an instance of D
from instance variable @d of an instance of C
from instance variable @c of an instance of B
from instance variable @b of an instance of A
from test.rb:12:in '<main>' |
|
I like it. I guess the only nitpick I'd have would be that perhaps all that should be in |
|
Yeah I'm not sure, it seems |
|
That's only on the error path though.
Yeah, I already suggested storing it in the exception and surfacing it via |
eceb099 to
bd6d236
Compare
|
I've made this script to check the allocations: class A; attr_accessor :b; end
class B; attr_accessor :c; end
class C; attr_accessor :d; end
class D; attr_accessor :e; end
def a(leaf = -> {})
a = A.new
a.b = b = B.new
b.c = c = C.new
c.d = d = D.new
d.e = leaf
a
end
def foo(desc, obj)
before = GC.stat(:total_allocated_objects)
Ractor.make_shareable(obj) rescue nil
puts "#{desc}: #{GC.stat(:total_allocated_objects) - before}"
end
foo "warmup no traversal", nil
foo "no traversal", nil
foo "warmup no traversal not shareable", ->{}
foo "no traversal not shareable", ->{}
foo "warmup shareable", a(nil)
foo "shareable", a(nil)
foo "warmup not shareable", a
foo "not shareable", aOn main: On this branch: We can see when the object is shareable, we still only allocate |
When an object fails to be made shareable with `Ractor.make_shareable` or when an unshareable object is accessed through module constants or module instance variables, the error message now includes the chain of references that leads to the unshareable value.
bd6d236 to
d00928a
Compare
When an object fails to be made shareable with
Ractor.make_shareableor when an unshareable object is accessed through module constants or module instance variables, the error message now includes the chain of references that leads to the unshareable value.For example with this code:
Currently you get:
This PR turns the error message into:
I chose not to display individual objects except in the "block self" case, but maybe we shouldn't even in that case (in this examplemainis pretty short). Including the objects'#inspectoutput causes the lines to easily become super long and the overall output to be messy.