From b845167e1185024e331f9985ad090dd1e4f33894 Mon Sep 17 00:00:00 2001 From: Julien Portalier Date: Tue, 21 Jan 2025 17:25:16 +0100 Subject: [PATCH 1/3] Reference#exec_recursive[_clone] methods aren't fiber aware MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- src/fiber.cr | 34 +++++++++++++++++++++++++++ src/reference.cr | 60 ++---------------------------------------------- 2 files changed, 36 insertions(+), 58 deletions(-) diff --git a/src/fiber.cr b/src/fiber.cr index b34a8762037d..39ca9c278074 100644 --- a/src/fiber.cr +++ b/src/fiber.cr @@ -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) @@ -331,4 +335,34 @@ 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 + # 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 + yield + hash.delete(key) + return true + end + 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 diff --git a/src/reference.cr b/src/reference.cr index f70697282fa0..798831e982f1 100644 --- a/src/reference.cr +++ b/src/reference.cr @@ -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: # @@ -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. @@ -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 From f36a0362a23cda5d8dc3cb03dde2e80192c0c12e Mon Sep 17 00:00:00 2001 From: Julien Portalier Date: Tue, 21 Jan 2025 17:46:28 +0100 Subject: [PATCH 2/3] fixup! Reference#exec_recursive[_clone] methods aren't fiber aware --- src/fiber.cr | 14 +++++++------- src/reference.cr | 3 ++- 2 files changed, 9 insertions(+), 8 deletions(-) diff --git a/src/fiber.cr b/src/fiber.cr index 39ca9c278074..417e5710c64c 100644 --- a/src/fiber.cr +++ b/src/fiber.cr @@ -339,7 +339,7 @@ class Fiber # :nodoc: # # Protects `Reference#inspect` and `Reference#pretty_print` against recursion. - def exec_recursive(object_id : UInt64, method : Symbol, &) : Bool + def exec_recursive(object_id : UInt64, method : Symbol, &) # NOTE: can't use `Set` because of prelude require order hash = (@exec_recursive ||= Hash({UInt64, Symbol}, Nil).new) key = {object_id, method} @@ -355,14 +355,14 @@ class Fiber # # Protects `Reference#clone` implementations against recursion. See # `Reference#exec_recursive_clone` for more details. - def exec_recursive_clone(object_id : UInt64, &) : Bool + def exec_recursive_clone(object_id : UInt64, &) # 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 = (@exec_recursive_clone ||= Hash(UInt64, UInt64).new) + clone_object_id = hash[object_id]? + unless clone_object_id + clone_object_id = yield(hash).object_id hash.delete(object_id) - return true end - false + Pointer(Void).new(clone_object_id) end end diff --git a/src/reference.cr b/src/reference.cr index 798831e982f1..dd6df1cfed98 100644 --- a/src/reference.cr +++ b/src/reference.cr @@ -199,6 +199,7 @@ class Reference # end # ``` private def exec_recursive_clone(&) - Fiber.current.exec_recursive_clone(object_id) { yield } + pointer = Fiber.current.exec_recursive_clone(object_id) { |hash| yield hash } + pointer.as(self) end end From 73c8b3ff4e1ffc7e4ee7499de9f6a7d732222306 Mon Sep 17 00:00:00 2001 From: Julien Portalier Date: Wed, 22 Jan 2025 10:25:04 +0100 Subject: [PATCH 3/3] Fix: don't move implementations to Fiber, only the hashes --- src/fiber.cr | 32 ++++++++------------------------ src/reference.cr | 20 +++++++++++++++++--- 2 files changed, 25 insertions(+), 27 deletions(-) diff --git a/src/fiber.cr b/src/fiber.cr index 417e5710c64c..b27c34fd6b36 100644 --- a/src/fiber.cr +++ b/src/fiber.cr @@ -163,8 +163,8 @@ class Fiber @timeout_select_action = nil # Additional cleanup (avoid stale references) - @exec_recursive = nil - @exec_recursive_clone = nil + @exec_recursive_hash = nil + @exec_recursive_clone_hash = nil @alive = false {% unless flag?(:interpreted) %} @@ -338,31 +338,15 @@ class Fiber # :nodoc: # - # Protects `Reference#inspect` and `Reference#pretty_print` against recursion. - def exec_recursive(object_id : UInt64, method : Symbol, &) - # 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 - yield - hash.delete(key) - return true - end - false + # See `Reference#exec_recursive` for details. + def exec_recursive_hash + @exec_recursive_hash ||= Hash({UInt64, Symbol}, Nil).new end # :nodoc: # - # Protects `Reference#clone` implementations against recursion. See - # `Reference#exec_recursive_clone` for more details. - def exec_recursive_clone(object_id : UInt64, &) - # NOTE: can't use `Set` because of prelude require order - hash = (@exec_recursive_clone ||= Hash(UInt64, UInt64).new) - 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) + # See `Reference#exec_recursive_clone` for details. + def exec_recursive_clone_hash + @exec_recursive_clone_hash ||= Hash(UInt64, UInt64).new end end diff --git a/src/reference.cr b/src/reference.cr index dd6df1cfed98..42bdcba2327a 100644 --- a/src/reference.cr +++ b/src/reference.cr @@ -177,7 +177,15 @@ class Reference end private def exec_recursive(method, &) - Fiber.current.exec_recursive(object_id, method) { yield } + # 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 + hash.delete(key) + return true + end + false end # Helper method to perform clone by also checking recursiveness. @@ -199,7 +207,13 @@ class Reference # end # ``` private def exec_recursive_clone(&) - pointer = Fiber.current.exec_recursive_clone(object_id) { |hash| yield hash } - pointer.as(self) + # 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 + hash.delete(object_id) + end + Pointer(Void).new(clone_object_id).as(self) end end