Skip to content

Commit

Permalink
Reference#exec_recursive[_clone] methods aren't fiber aware
Browse files Browse the repository at this point in the history
Recursion of the `#inspect` and `#pretty_print` methods is handled by a
global hash, which is a per-thread global, but if any of these methods
yield —which will happen because they're writing to an IO— another
fiber running on the same thread will falsely detect a recursion if it
inspects the same object.

This patch moves `Reference#exec_recursive` methods as instances of
`Fiber`, using a per-fiber hash instead of per-thread/process hash.

References #15088
  • Loading branch information
ysbaddaden committed Jan 21, 2025
1 parent 5a245d9 commit b8ab18c
Show file tree
Hide file tree
Showing 2 changed files with 43 additions and 58 deletions.
41 changes: 41 additions & 0 deletions src/fiber.cr
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,10 @@ class Fiber
@timeout_event.try &.free
@timeout_select_action = nil

# Additional cleanup (avoid stale references)
@exec_recursive = nil
@exec_recursive_clone = nil

@alive = false
{% unless flag?(:interpreted) %}
Crystal::Scheduler.stack_pool.release(@stack)
Expand Down Expand Up @@ -331,4 +335,41 @@ class Fiber
@current_thread.lazy_get
end
{% end %}

# :nodoc:
#
# Protects `Reference#inspect` and `Reference#pretty_print` against recursion.
def exec_recursive(object_id : UInt64, method : Symbol, &) : Bool
LibC.dprintf 2, "#{Fiber.current.name}: #{self.object_id}#exec_recursive(#{object_id}, #{method})\n"

# NOTE: can't use `Set` because of prelude require order
hash = (@exec_recursive ||= Hash({UInt64, Symbol}, Nil).new)

key = {object_id, method}
hash.put(key, nil) do
LibC.dprintf 2, "#{Fiber.current.name}: #{self.object_id}#exec_recursive(#{object_id}, #{method}): yield\n"
yield
hash.delete(key)
LibC.dprintf 2, "#{Fiber.current.name}: #{self.object_id}#exec_recursive(#{object_id}, #{method}): done\n"
return true
end

LibC.dprintf 2, "#{Fiber.current.name}: #{self.object_id}#exec_recursive(#{object_id}, #{method}): done\n"
false
end

# :nodoc:
#
# Protects `Reference#clone` implementations against recursion. See
# `Reference#exec_recursive_clone` for more details.
def exec_recursive_clone(object_id : UInt64, &) : Bool
# NOTE: can't use `Set` because of prelude require order
hash = (@exec_recursive_clone ||= Hash(UInt64, Nil).new)
hash.put(object_id, nil) do
yield
hash.delete(object_id)
return true
end
false
end
end
60 changes: 2 additions & 58 deletions src/reference.cr
Original file line number Diff line number Diff line change
@@ -1,7 +1,3 @@
{% if flag?(:preview_mt) %}
require "crystal/thread_local_value"
{% end %}

# `Reference` is the base class of classes you define in your program.
# It is set as a class' superclass when you don't specify one:
#
Expand Down Expand Up @@ -180,54 +176,8 @@ class Reference
io << '>'
end

# :nodoc:
module ExecRecursive
# NOTE: can't use `Set` here because of prelude require order
alias Registry = Hash({UInt64, Symbol}, Nil)

{% if flag?(:preview_mt) %}
@@exec_recursive = Crystal::ThreadLocalValue(Registry).new
{% else %}
@@exec_recursive = Registry.new
{% end %}

def self.hash
{% if flag?(:preview_mt) %}
@@exec_recursive.get { Registry.new }
{% else %}
@@exec_recursive
{% end %}
end
end

private def exec_recursive(method, &)
hash = ExecRecursive.hash
key = {object_id, method}
hash.put(key, nil) do
yield
hash.delete(key)
return true
end
false
end

# :nodoc:
module ExecRecursiveClone
alias Registry = Hash(UInt64, UInt64)

{% if flag?(:preview_mt) %}
@@exec_recursive = Crystal::ThreadLocalValue(Registry).new
{% else %}
@@exec_recursive = Registry.new
{% end %}

def self.hash
{% if flag?(:preview_mt) %}
@@exec_recursive.get { Registry.new }
{% else %}
@@exec_recursive
{% end %}
end
Fiber.current.exec_recursive(object_id, method) { yield }
end

# Helper method to perform clone by also checking recursiveness.
Expand All @@ -249,12 +199,6 @@ class Reference
# end
# ```
private def exec_recursive_clone(&)
hash = ExecRecursiveClone.hash
clone_object_id = hash[object_id]?
unless clone_object_id
clone_object_id = yield(hash).object_id
hash.delete(object_id)
end
Pointer(Void).new(clone_object_id).as(self)
Fiber.current.exec_recursive_clone(object_id) { yield }
end
end

0 comments on commit b8ab18c

Please sign in to comment.