Skip to content

Commit

Permalink
Fix Reference#exec_recursive, #exec_recursive_clone to be fiber a…
Browse files Browse the repository at this point in the history
…ware (#15361)

Recursion of the `#inspect`, `#pretty_print` and `#clone` methods is handled
by a per-thread global hash. But if any of these methods
yields —which will happen when 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 the backing hashes to instance variables of
`Fiber`, using a per-fiber hash instead of per-thread/process hash.

References #15088
  • Loading branch information
ysbaddaden authored Jan 23, 2025
1 parent 35ecb7d commit df06679
Show file tree
Hide file tree
Showing 2 changed files with 22 additions and 45 deletions.
18 changes: 18 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_hash = nil
@exec_recursive_clone_hash = nil

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

# :nodoc:
#
# See `Reference#exec_recursive` for details.
def exec_recursive_hash
@exec_recursive_hash ||= Hash({UInt64, Symbol}, Nil).new
end

# :nodoc:
#
# See `Reference#exec_recursive_clone` for details.
def exec_recursive_clone_hash
@exec_recursive_clone_hash ||= Hash(UInt64, UInt64).new
end
end
49 changes: 4 additions & 45 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,28 +176,9 @@ 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
# NOTE: can't use `Set` because of prelude require order
hash = Fiber.current.exec_recursive_hash
key = {object_id, method}
hash.put(key, nil) do
yield
Expand All @@ -211,25 +188,6 @@ class Reference
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
end

# Helper method to perform clone by also checking recursiveness.
# When clone is wanted, call this method. Then create the clone
# instance without any contents (don't fill it out yet), then
Expand All @@ -249,7 +207,8 @@ class Reference
# end
# ```
private def exec_recursive_clone(&)
hash = ExecRecursiveClone.hash
# NOTE: can't use `Set` because of prelude require order
hash = Fiber.current.exec_recursive_clone_hash
clone_object_id = hash[object_id]?
unless clone_object_id
clone_object_id = yield(hash).object_id
Expand Down

0 comments on commit df06679

Please sign in to comment.