From 060c6cfbf1848015542971de29b3f9c53d9fa231 Mon Sep 17 00:00:00 2001 From: Julien Portalier Date: Mon, 13 Jan 2025 15:02:06 +0100 Subject: [PATCH 1/7] Fix: initializing constants and class vars is concurrency unsafe --- src/crystal/once.cr | 68 +++++++++++++++------------------------------ 1 file changed, 22 insertions(+), 46 deletions(-) diff --git a/src/crystal/once.cr b/src/crystal/once.cr index 7b6c7e6ed0f2..027e3093befc 100644 --- a/src/crystal/once.cr +++ b/src/crystal/once.cr @@ -7,12 +7,8 @@ # - `__crystal_once`: called each time a constant or class variable has to be # initialized and is its responsibility to verify the initializer is executed # only once and to fail on recursion. - -# In multithread mode a mutex is used to avoid race conditions between threads. # -# On Win32, `Crystal::System::FileDescriptor#@@reader_thread` spawns a new -# thread even without the `preview_mt` flag, and the thread can also reference -# Crystal constants, leading to race conditions, so we always enable the mutex. +# A `Mutex` is used to avoid race conditions between threads and fibers. {% if compare_versions(Crystal::VERSION, "1.16.0-dev") >= 0 %} # This implementation uses an enum over the initialization flag pointer for @@ -26,24 +22,29 @@ Initialized = 1 end - {% if flag?(:preview_mt) || flag?(:win32) %} - @@once_mutex = uninitialized Mutex + @@once_mutex = uninitialized Mutex - # :nodoc: - def self.once_mutex=(@@once_mutex : Mutex) - end - {% end %} + # :nodoc: + def self.once_mutex=(@@once_mutex : Mutex) + end # :nodoc: # Using @[NoInline] so LLVM optimizes for the hot path (var already # initialized). @[NoInline] def self.once(flag : OnceState*, initializer : Void*) : Nil - {% if flag?(:preview_mt) || flag?(:win32) %} - @@once_mutex.synchronize { once_exec(flag, initializer) } - {% else %} - once_exec(flag, initializer) - {% end %} + @@once_mutex.synchronize do + case flag.value + in .initialized? + return + in .uninitialized? + flag.value = :processing + Proc(Nil).new(initializer, Pointer(Void).null).call + flag.value = :initialized + in .processing? + raise "Recursion while initializing class variables and/or constants" + end + end # safety check, and allows to safely call `Intrinsics.unreachable` in # `__crystal_once` @@ -52,26 +53,11 @@ LibC._exit(1) end end - - private def self.once_exec(flag : OnceState*, initializer : Void*) : Nil - case flag.value - in .initialized? - return - in .uninitialized? - flag.value = :processing - Proc(Nil).new(initializer, Pointer(Void).null).call - flag.value = :initialized - in .processing? - raise "Recursion while initializing class variables and/or constants" - end - end end # :nodoc: fun __crystal_once_init : Nil - {% if flag?(:preview_mt) || flag?(:win32) %} - Crystal.once_mutex = Mutex.new(:reentrant) - {% end %} + Crystal.once_mutex = Mutex.new(:reentrant) end # :nodoc: @@ -96,11 +82,14 @@ # :nodoc: class Crystal::OnceState + @mutex = Mutex.new(:reentrant) @rec = [] of Bool* @[NoInline] def once(flag : Bool*, initializer : Void*) - unless flag.value + return if flag.value + + @mutex.synchronize do if @rec.includes?(flag) raise "Recursion while initializing class variables and/or constants" end @@ -112,19 +101,6 @@ @rec.pop end end - - {% if flag?(:preview_mt) || flag?(:win32) %} - @mutex = Mutex.new(:reentrant) - - @[NoInline] - def once(flag : Bool*, initializer : Void*) - unless flag.value - @mutex.synchronize do - previous_def - end - end - end - {% end %} end # :nodoc: From 86ff3c4ff31366a09c814aa724899aecb5b47580 Mon Sep 17 00:00:00 2001 From: Julien Portalier Date: Mon, 13 Jan 2025 15:02:41 +0100 Subject: [PATCH 2/7] Fix: don't use lazy class getter/property helpers in Thread and Fiber We need a Mutex to protect against recursion and to make sure the lazy initializers only run once, but Mutex depends on the current fiber, and indirectly the current thread, which themselves may not have been initialized yet and will lead to an infinite recursion once we protect the class getter and property helpers. --- src/crystal/once.cr | 4 ++++ src/crystal/system/thread.cr | 13 ++++++++++++- src/crystal/system/unix/pthread.cr | 29 ++++++++++++++++++++--------- src/crystal/system/wasi/thread.cr | 14 +++++++++++++- src/crystal/system/win32/thread.cr | 27 +++++++++++++++++++-------- src/fiber.cr | 10 +++++++++- 6 files changed, 77 insertions(+), 20 deletions(-) diff --git a/src/crystal/once.cr b/src/crystal/once.cr index 027e3093befc..4b60d9c43534 100644 --- a/src/crystal/once.cr +++ b/src/crystal/once.cr @@ -57,6 +57,8 @@ # :nodoc: fun __crystal_once_init : Nil + Thread.init + Fiber.init Crystal.once_mutex = Mutex.new(:reentrant) end @@ -105,6 +107,8 @@ # :nodoc: fun __crystal_once_init : Void* + Thread.init + Fiber.init Crystal::OnceState.new.as(Void*) end diff --git a/src/crystal/system/thread.cr b/src/crystal/system/thread.cr index 92136d1f3989..878a27e4c578 100644 --- a/src/crystal/system/thread.cr +++ b/src/crystal/system/thread.cr @@ -2,6 +2,8 @@ module Crystal::System::Thread # alias Handle + # def self.init : Nil + # def self.new_handle(thread_obj : ::Thread) : Handle # def self.current_handle : Handle @@ -48,7 +50,16 @@ class Thread include Crystal::System::Thread # all thread objects, so the GC can see them (it doesn't scan thread locals) - protected class_getter(threads) { Thread::LinkedList(Thread).new } + @@threads = uninitialized Thread::LinkedList(Thread) + + protected def self.threads : Thread::LinkedList(Thread) + @@threads + end + + def self.init : Nil + @@threads = Thread::LinkedList(Thread).new + Crystal::System::Thread.init + end @system_handle : Crystal::System::Thread::Handle @exception : Exception? diff --git a/src/crystal/system/unix/pthread.cr b/src/crystal/system/unix/pthread.cr index 98629a70fbb6..e91990689084 100644 --- a/src/crystal/system/unix/pthread.cr +++ b/src/crystal/system/unix/pthread.cr @@ -26,6 +26,16 @@ module Crystal::System::Thread raise RuntimeError.from_os_error("pthread_create", Errno.new(ret)) unless ret == 0 end + def self.init : Nil + {% if flag?(:musl) %} + @@main_handle = current_handle + {% elsif flag?(:openbsd) || flag?(:android) %} + ret = LibC.pthread_key_create(out current_key, nil) + raise RuntimeError.from_os_error("pthread_key_create", Errno.new(ret)) unless ret == 0 + @@current_key = current_key + {% end %} + end + def self.thread_proc(data : Void*) : Void* th = data.as(::Thread) @@ -53,13 +63,7 @@ module Crystal::System::Thread # Android appears to support TLS to some degree, but executables fail with # an underaligned TLS segment, see https://github.com/crystal-lang/crystal/issues/13951 {% if flag?(:openbsd) || flag?(:android) %} - @@current_key : LibC::PthreadKeyT - - @@current_key = begin - ret = LibC.pthread_key_create(out current_key, nil) - raise RuntimeError.from_os_error("pthread_key_create", Errno.new(ret)) unless ret == 0 - current_key - end + @@current_key = uninitialized LibC::PthreadKeyT def self.current_thread : ::Thread if ptr = LibC.pthread_getspecific(@@current_key) @@ -84,11 +88,18 @@ module Crystal::System::Thread end {% else %} @[ThreadLocal] - class_property current_thread : ::Thread { ::Thread.new } + @@current_thread : ::Thread? + + def self.current_thread : ::Thread + @@current_thread ||= ::Thread.new + end def self.current_thread? : ::Thread? @@current_thread end + + def self.current_thread=(@@current_thread : ::Thread) + end {% end %} def self.sleep(time : ::Time::Span) : Nil @@ -169,7 +180,7 @@ module Crystal::System::Thread end {% if flag?(:musl) %} - @@main_handle : Handle = current_handle + @@main_handle = uninitialized Handle def self.current_is_main? current_handle == @@main_handle diff --git a/src/crystal/system/wasi/thread.cr b/src/crystal/system/wasi/thread.cr index 1e8f6957d526..d103c7d9fc44 100644 --- a/src/crystal/system/wasi/thread.cr +++ b/src/crystal/system/wasi/thread.cr @@ -1,6 +1,9 @@ module Crystal::System::Thread alias Handle = Nil + def self.init : Nil + end + def self.new_handle(thread_obj : ::Thread) : Handle raise NotImplementedError.new("Crystal::System::Thread.new_handle") end @@ -13,7 +16,16 @@ module Crystal::System::Thread raise NotImplementedError.new("Crystal::System::Thread.yield_current") end - class_property current_thread : ::Thread { ::Thread.new } + def self.current_thread : ::Thread + @@current_thread ||= ::Thread.new + end + + def self.current_thread? : ::Thread? + @@current_thread + end + + def self.current_thread=(@@current_thread : ::Thread) + end def self.sleep(time : ::Time::Span) : Nil req = uninitialized LibC::Timespec diff --git a/src/crystal/system/win32/thread.cr b/src/crystal/system/win32/thread.cr index 9cb60f01ced8..2ff7ca438d87 100644 --- a/src/crystal/system/win32/thread.cr +++ b/src/crystal/system/win32/thread.cr @@ -20,6 +20,16 @@ module Crystal::System::Thread ) end + def self.init : Nil + {% if flag?(:gnu) %} + current_key = LibC.TlsAlloc + if current_key == LibC::TLS_OUT_OF_INDEXES + Crystal::System.panic("TlsAlloc()", WinError.value) + end + @@current_key = current_key + {% end %} + end + def self.thread_proc(data : Void*) : LibC::UInt # ensure that even in the case of stack overflow there is enough reserved # stack space for recovery (for the main thread this is done in @@ -47,13 +57,7 @@ module Crystal::System::Thread # MinGW does not support TLS correctly {% if flag?(:gnu) %} - @@current_key : LibC::DWORD = begin - current_key = LibC.TlsAlloc - if current_key == LibC::TLS_OUT_OF_INDEXES - Crystal::System.panic("TlsAlloc()", WinError.value) - end - current_key - end + @@current_key = uninitialized LibC::DWORD def self.current_thread : ::Thread th = current_thread? @@ -82,11 +86,18 @@ module Crystal::System::Thread end {% else %} @[ThreadLocal] - class_property current_thread : ::Thread { ::Thread.new } + @@current_thread : ::Thread? + + def self.current_thread : ::Thread + @@current_thread ||= ::Thread.new + end def self.current_thread? : ::Thread? @@current_thread end + + def self.current_thread=(@@current_thread : ::Thread) + end {% end %} def self.sleep(time : ::Time::Span) : Nil diff --git a/src/fiber.cr b/src/fiber.cr index b34a8762037d..c56effd57780 100644 --- a/src/fiber.cr +++ b/src/fiber.cr @@ -44,8 +44,16 @@ end # notifications that IO is ready or a timeout reached. When a fiber can be woken, # the event loop enqueues it in the scheduler class Fiber + @@fibers = uninitialized Thread::LinkedList(Fiber) + + protected def self.fibers : Thread::LinkedList(Fiber) + @@fibers + end + # :nodoc: - protected class_getter(fibers) { Thread::LinkedList(Fiber).new } + def self.init : Nil + @@fibers = Thread::LinkedList(Fiber).new + end @context : Context @stack : Void* From f08ef9362113722988f122ece5d5265106db318e Mon Sep 17 00:00:00 2001 From: Julien Portalier Date: Mon, 13 Jan 2025 16:10:01 +0100 Subject: [PATCH 3/7] Add `Crystal.once` to protect classvar lazy initialization Reuses the logic for `__crystal_once`. --- src/crystal/once.cr | 71 ++++++++++++++++++++++++++++++++------------- 1 file changed, 51 insertions(+), 20 deletions(-) diff --git a/src/crystal/once.cr b/src/crystal/once.cr index 4b60d9c43534..ee52a7d9b54f 100644 --- a/src/crystal/once.cr +++ b/src/crystal/once.cr @@ -8,6 +8,9 @@ # initialized and is its responsibility to verify the initializer is executed # only once and to fail on recursion. # +# Also defines the `Crystal.once(flag, &)` method used to protect lazy +# initialization of class getters & properties. +# # A `Mutex` is used to avoid race conditions between threads and fibers. {% if compare_versions(Crystal::VERSION, "1.16.0-dev") >= 0 %} @@ -28,18 +31,28 @@ def self.once_mutex=(@@once_mutex : Mutex) end + # :nodoc: + # Identical to `__crystal_once` but takes a block with possibly closured + # data. Used by `class_[getter|property](declaration, &block)` for example. + @[AlwaysInline] + def self.once(flag : OnceState*, &block) : Nil + return if flag.value.initialized? + once(flag, block.pointer, block.closure_data) + Intrinsics.unreachable unless flag.value.initialized? + end + # :nodoc: # Using @[NoInline] so LLVM optimizes for the hot path (var already # initialized). @[NoInline] - def self.once(flag : OnceState*, initializer : Void*) : Nil + def self.once(flag : OnceState*, initializer : Void*, closure_data : Void*) : Nil @@once_mutex.synchronize do case flag.value in .initialized? return in .uninitialized? flag.value = :processing - Proc(Nil).new(initializer, Pointer(Void).null).call + Proc(Nil).new(initializer, closure_data).call flag.value = :initialized in .processing? raise "Recursion while initializing class variables and/or constants" @@ -71,7 +84,7 @@ fun __crystal_once(flag : Crystal::OnceState*, initializer : Void*) : Nil return if flag.value.initialized? - Crystal.once(flag, initializer) + Crystal.once(flag, initializer, Pointer(Void).null) # tell LLVM that it can optimize away repeated `__crystal_once` calls for # this global (e.g. repeated access to constant in a single funtion); @@ -82,41 +95,59 @@ # This implementation uses a global array to store the initialization flag # pointers for each value to find infinite loops and raise an error. - # :nodoc: - class Crystal::OnceState - @mutex = Mutex.new(:reentrant) - @rec = [] of Bool* + module Crystal + # :nodoc: + class OnceState + @mutex = Mutex.new(:reentrant) + @rec = [] of Bool* - @[NoInline] - def once(flag : Bool*, initializer : Void*) - return if flag.value + @[NoInline] + def once(flag : Bool*, initializer : Void*, closure_data : Void*) + return if flag.value - @mutex.synchronize do - if @rec.includes?(flag) - raise "Recursion while initializing class variables and/or constants" - end - @rec << flag + @mutex.synchronize do + return if flag.value - Proc(Nil).new(initializer, Pointer(Void).null).call - flag.value = true + if @rec.includes?(flag) + raise "Recursion while initializing class variables and/or constants" + end + @rec << flag - @rec.pop + Proc(Nil).new(initializer, closure_data).call + flag.value = true + + @rec.pop + end end end + + @@once_state = uninitialized OnceState + + # :nodoc: + def self.once_state=(@@once_state : OnceState) + end + + # :nodoc: + @[AlwaysInline] + def self.once(flag : Bool*, &block) : Nil + return if flag.value + @@once_state.once(flag, block.pointer, block.closure_data) + Intrinsics.unreachable unless flag.value + end end # :nodoc: fun __crystal_once_init : Void* Thread.init Fiber.init - Crystal::OnceState.new.as(Void*) + (Crystal.once_state = Crystal::OnceState.new).as(Void*) end # :nodoc: @[AlwaysInline] fun __crystal_once(state : Void*, flag : Bool*, initializer : Void*) return if flag.value - state.as(Crystal::OnceState).once(flag, initializer) + state.as(Crystal::OnceState).once(flag, initializer, Pointer(Void).null) Intrinsics.unreachable unless flag.value end {% end %} From dc635fb4cdafa055b276c15e52a260ca890990f6 Mon Sep 17 00:00:00 2001 From: Julien Portalier Date: Mon, 13 Jan 2025 16:11:28 +0100 Subject: [PATCH 4/7] Use Crystal.once in lazy initializers Protects against recursion and adds thread (parallelism) and fiber (concurrency) safety to class var initialization. --- src/object.cr | 288 ++++++++++++++++++++++++++++++++++++++++---------- 1 file changed, 232 insertions(+), 56 deletions(-) diff --git a/src/object.cr b/src/object.cr index 4443eaec3916..40207aa3a037 100644 --- a/src/object.cr +++ b/src/object.cr @@ -432,7 +432,11 @@ class Object # end # ``` # + # {% if macro_prefix == "class_" %} + # Is similar to writing (thread and fiber safety omitted): + # {% else %} # Is the same as writing: + # {% end %} # # ``` # class Person @@ -448,29 +452,71 @@ class Object macro {{macro_prefix}}getter(*names, &block) \{% if block %} \{% if names.size != 1 %} - \{{ raise "Only one argument can be passed to `getter` with a block" }} + \{{ raise "Only one argument can be passed to `#{macro_prefix}getter` with a block" }} \{% end %} \{% name = names[0] %} \{% if name.is_a?(TypeDeclaration) %} - {{var_prefix}}\{{name.var.id}} : \{{name.type}}? + {% if macro_prefix == "class_" %} + {{var_prefix}}\{{name.var.id}} : \{{name.type}}? - def {{method_prefix}}\{{name.var.id}} : \{{name.type}} - if (%value = {{var_prefix}}\{{name.var.id}}).nil? - {{var_prefix}}\{{name.var.id}} = \{{yield}} - else - %value + {% if compare_versions(Crystal::VERSION, "1.16.0-dev") >= 0 %} + {{var_prefix}}__\{{name.var.id}}_once_flag : ::Crystal::OnceState = :uninitialized + {% else %} + {{var_prefix}}__\{{name.var.id}}_once_flag : Bool = false + {% end %} + + def {{method_prefix}}\{{name.var.id}} : \{{name.type}} + if %value = {{var_prefix}}\{{name.var.id}} + return %value + end + ::Crystal.once(pointerof({{var_prefix}}__\{{name.var.id}}_once_flag)) do + if {{var_prefix}}\{{name.var.id}}.nil? + {{var_prefix}}\{{name.var.id}} = \{{yield}} + end + end + {{var_prefix}}\{{name.var.id}}.not_nil! end - end + {% else %} + {{var_prefix}}\{{name.var.id}} : \{{name.type}}? + + def {{method_prefix}}\{{name.var.id}} : \{{name.type}} + if (%value = {{var_prefix}}\{{name.var.id}}).nil? + {{var_prefix}}\{{name.var.id}} = \{{yield}} + else + %value + end + end + {% end %} \{% else %} - def {{method_prefix}}\{{name.id}} - if (%value = {{var_prefix}}\{{name.id}}).nil? - {{var_prefix}}\{{name.id}} = \{{yield}} - else - %value + {% if macro_prefix == "class_" %} + {% if compare_versions(Crystal::VERSION, "1.16.0-dev") >= 0 %} + {{var_prefix}}__\{{name.id}}_once_flag : ::Crystal::OnceState = :uninitialized + {% else %} + {{var_prefix}}__\{{name.id}}_once_flag : Bool = false + {% end %} + + def {{method_prefix}}\{{name.id}} + if %value = {{var_prefix}}\{{name.id}} + return %value + end + ::Crystal.once(pointerof({{var_prefix}}__\{{name.id}}_once_flag)) do + if {{var_prefix}}\{{name.id}}.nil? + {{var_prefix}}\{{name.id}} = \{{yield}} + end + end + {{var_prefix}}\{{name.id}}.not_nil! end - end + {% else %} + def {{method_prefix}}\{{name.id}} + if (%value = {{var_prefix}}\{{name.id}}).nil? + {{var_prefix}}\{{name.id}} = \{{yield}} + else + %value + end + end + {% end %} \{% end %} \{% else %} \{% for name in names %} @@ -679,29 +725,71 @@ class Object macro {{macro_prefix}}getter?(*names, &block) \{% if block %} \{% if names.size != 1 %} - \{{ raise "Only one argument can be passed to `getter?` with a block" }} + \{{ raise "Only one argument can be passed to `#{macro_prefix}getter?` with a block" }} \{% end %} \{% name = names[0] %} \{% if name.is_a?(TypeDeclaration) %} - {{var_prefix}}\{{name.var.id}} : \{{name.type}}? + {% if macro_prefix == "class_" %} + {{var_prefix}}\{{name.var.id}} : \{{name.type}}? - def {{method_prefix}}\{{name.var.id}}? : \{{name.type}} - if (%value = {{var_prefix}}\{{name.var.id}}).nil? - {{var_prefix}}\{{name.var.id}} = \{{yield}} - else - %value + {% if compare_versions(Crystal::VERSION, "1.16.0-dev") >= 0 %} + {{var_prefix}}__\{{name.var.id}}_once_flag : ::Crystal::OnceState = :uninitialized + {% else %} + {{var_prefix}}__\{{name.var.id}}_once_flag : Bool = false + {% end %} + + def {{method_prefix}}\{{name.var.id}}? : \{{name.type}} + if %value = {{var_prefix}}\{{name.var.id}} + return %value + end + ::Crystal.once(pointerof({{var_prefix}}__\{{name.var.id}}_once_flag)) do + if {{var_prefix}}\{{name.var.id}}.nil? + {{var_prefix}}\{{name.var.id}} = \{{yield}} + end + end + {{var_prefix}}\{{name.var.id}}.not_nil! end - end + {% else %} + {{var_prefix}}\{{name.var.id}} : \{{name.type}}? + + def {{method_prefix}}\{{name.var.id}}? : \{{name.type}} + if (%value = {{var_prefix}}\{{name.var.id}}).nil? + {{var_prefix}}\{{name.var.id}} = \{{yield}} + else + %value + end + end + {% end %} \{% else %} - def {{method_prefix}}\{{name.id}}? - if (%value = {{var_prefix}}\{{name.id}}).nil? - {{var_prefix}}\{{name.id}} = \{{yield}} - else - %value + {% if macro_prefix == "class_" %} + {% if compare_versions(Crystal::VERSION, "1.16.0-dev") >= 0 %} + {{var_prefix}}__\{{name.id}}_once_flag : ::Crystal::OnceState = :uninitialized + {% else %} + {{var_prefix}}__\{{name.id}}_once_flag : Bool = false + {% end %} + + def {{method_prefix}}\{{name.id}}? + if %value = {{var_prefix}}\{{name.id}} + return %value + end + ::Crystal.once(pointerof({{var_prefix}}__\{{name.id}}_once_flag)) do + if {{var_prefix}}\{{name.id}}.nil? + {{var_prefix}}\{{name.id}} = \{{yield}} + end + end + {{var_prefix}}\{{name.id}}.not_nil! end - end + {% else %} + def {{method_prefix}}\{{name.id}}? + if (%value = {{var_prefix}}\{{name.id}}).nil? + {{var_prefix}}\{{name.id}} = \{{yield}} + else + %value + end + end + {% end %} \{% end %} \{% else %} \{% for name in names %} @@ -942,7 +1030,11 @@ class Object # end # ``` # + # {% if macro_prefix == "class_" %} + # Is similar to writing (thread and fiber safety omitted): + # {% else %} # Is the same as writing: + # {% end %} # # ``` # class Person @@ -961,32 +1053,74 @@ class Object macro {{macro_prefix}}property(*names, &block) \{% if block %} \{% if names.size != 1 %} - \{{ raise "Only one argument can be passed to `property` with a block" }} + \{{ raise "Only one argument can be passed to `#{macro_prefix}property` with a block" }} \{% end %} \{% name = names[0] %} \{% if name.is_a?(TypeDeclaration) %} - {{var_prefix}}\{{name.var.id}} : \{{name.type}}? + {% if macro_prefix == "class_" %} + {{var_prefix}}\{{name.var.id}} : \{{name.type}}? - def {{method_prefix}}\{{name.var.id}} : \{{name.type}} - if (%value = {{var_prefix}}\{{name.var.id}}).nil? - {{var_prefix}}\{{name.var.id}} = \{{yield}} - else - %value + {% if compare_versions(Crystal::VERSION, "1.16.0-dev") >= 0 %} + {{var_prefix}}__\{{name.var.id}}_once_flag : ::Crystal::OnceState = :uninitialized + {% else %} + {{var_prefix}}__\{{name.var.id}}_once_flag : Bool = false + {% end %} + + def {{method_prefix}}\{{name.var.id}} : \{{name.type}} + if %value = {{var_prefix}}\{{name.var.id}} + return %value + end + ::Crystal.once(pointerof({{var_prefix}}__\{{name.var.id}}_once_flag)) do + if {{var_prefix}}\{{name.var.id}}.nil? + {{var_prefix}}\{{name.var.id}} = \{{yield}} + end + end + {{var_prefix}}\{{name.var.id}}.not_nil! end - end + {% else %} + {{var_prefix}}\{{name.var.id}} : \{{name.type}}? + + def {{method_prefix}}\{{name.var.id}} : \{{name.type}} + if (%value = {{var_prefix}}\{{name.var.id}}).nil? + {{var_prefix}}\{{name.var.id}} = \{{yield}} + else + %value + end + end + {% end %} def {{method_prefix}}\{{name.var.id}}=({{var_prefix}}\{{name.var.id}} : \{{name.type}}) end \{% else %} - def {{method_prefix}}\{{name.id}} - if (%value = {{var_prefix}}\{{name.id}}).nil? - {{var_prefix}}\{{name.id}} = \{{yield}} - else - %value + {% if macro_prefix == "class_" %} + {% if compare_versions(Crystal::VERSION, "1.16.0-dev") >= 0 %} + {{var_prefix}}__\{{name.id}}_once_flag : ::Crystal::OnceState = :uninitialized + {% else %} + {{var_prefix}}__\{{name.id}}_once_flag : Bool = false + {% end %} + + def {{method_prefix}}\{{name.id}} + if %value = {{var_prefix}}\{{name.id}} + return %value + end + ::Crystal.once(pointerof({{var_prefix}}__\{{name.id}}_once_flag)) do + if {{var_prefix}}\{{name.id}}.nil? + {{var_prefix}}\{{name.id}} = \{{yield}} + end + end + {{var_prefix}}\{{name.id}}.not_nil! end - end + {% else %} + def {{method_prefix}}\{{name.id}} + if (%value = {{var_prefix}}\{{name.id}}).nil? + {{var_prefix}}\{{name.id}} = \{{yield}} + else + %value + end + end + {% end %} def {{method_prefix}}\{{name.id}}=({{var_prefix}}\{{name.id}}) end @@ -1207,32 +1341,74 @@ class Object macro {{macro_prefix}}property?(*names, &block) \{% if block %} \{% if names.size != 1 %} - \{{ raise "Only one argument can be passed to `property?` with a block" }} + \{{ raise "Only one argument can be passed to `#{macro_prefix}property?` with a block" }} \{% end %} \{% name = names[0] %} \{% if name.is_a?(TypeDeclaration) %} - {{var_prefix}}\{{name.var.id}} : \{{name.type}}? + {% if macro_prefix == "class_" %} + {{var_prefix}}\{{name.var.id}} : \{{name.type}}? - def {{method_prefix}}\{{name.var.id}}? : \{{name.type}} - if (%value = {{var_prefix}}\{{name.var.id}}).nil? - {{var_prefix}}\{{name.var.id}} = \{{yield}} - else - %value + {% if compare_versions(Crystal::VERSION, "1.16.0-dev") >= 0 %} + {{var_prefix}}__\{{name.var.id}}_once_flag : ::Crystal::OnceState = :uninitialized + {% else %} + {{var_prefix}}__\{{name.var.id}}_once_flag : Bool = false + {% end %} + + def {{method_prefix}}\{{name.var.id}}? : \{{name.type}} + if %value = {{var_prefix}}\{{name.var.id}} + return %value + end + ::Crystal.once(pointerof({{var_prefix}}__\{{name.var.id}}_once_flag)) do + if {{var_prefix}}\{{name.var.id}}.nil? + {{var_prefix}}\{{name.var.id}} = \{{yield}} + end + end + {{var_prefix}}\{{name.var.id}}.not_nil! end - end + {% else %} + {{var_prefix}}\{{name.var.id}} : \{{name.type}}? + + def {{method_prefix}}\{{name.var.id}}? : \{{name.type}} + if (%value = {{var_prefix}}\{{name.var.id}}).nil? + {{var_prefix}}\{{name.var.id}} = \{{yield}} + else + %value + end + end + {% end %} def {{method_prefix}}\{{name.var.id}}=({{var_prefix}}\{{name.var.id}} : \{{name.type}}) end \{% else %} - def {{method_prefix}}\{{name.id}}? - if (%value = {{var_prefix}}\{{name.id}}).nil? - {{var_prefix}}\{{name.id}} = \{{yield}} - else - %value + {% if macro_prefix == "class_" %} + {% if compare_versions(Crystal::VERSION, "1.16.0-dev") >= 0 %} + {{var_prefix}}__\{{name.id}}_once_flag : ::Crystal::OnceState = :uninitialized + {% else %} + {{var_prefix}}__\{{name.id}}_once_flag : Bool = false + {% end %} + + def {{method_prefix}}\{{name.id}}? + if %value = {{var_prefix}}\{{name.id}} + return %value + end + ::Crystal.once(pointerof({{var_prefix}}__\{{name.id}}_once_flag)) do + if {{var_prefix}}\{{name.id}}.nil? + {{var_prefix}}\{{name.id}} = \{{yield}} + end + end + {{var_prefix}}\{{name.id}}.not_nil! end - end + {% else %} + def {{method_prefix}}\{{name.id}}? + if (%value = {{var_prefix}}\{{name.id}}).nil? + {{var_prefix}}\{{name.id}} = \{{yield}} + else + %value + end + end + {% end %} def {{method_prefix}}\{{name.id}}=({{var_prefix}}\{{name.id}}) end From ab8b994875f61215852f45993399e332eae04491 Mon Sep 17 00:00:00 2001 From: Julien Portalier Date: Mon, 13 Jan 2025 16:12:24 +0100 Subject: [PATCH 5/7] Fix: SocketSpecHelper.supports_ipv6? The initializer block is now captured, and we can't return from a captured block. This outlines that the previous commit is a BREAKING CHANGE! The `class_getter?` version used skip over the intent to cache the result in `@@supports_ipv6` (it returned from the generated function, not from the block), so whenever IPv6 was supported every test was creating yet-another TCPServer (oops). --- spec/std/socket/spec_helper.cr | 6 +++++- spec/support/time.cr | 4 +++- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/spec/std/socket/spec_helper.cr b/spec/std/socket/spec_helper.cr index 276e2f4195f2..3f9480d46b6d 100644 --- a/spec/std/socket/spec_helper.cr +++ b/spec/std/socket/spec_helper.cr @@ -2,7 +2,11 @@ require "spec" require "socket" module SocketSpecHelper - class_getter?(supports_ipv6 : Bool) do + @@supports_ipv6 : Bool? + + class_getter?(supports_ipv6 : Bool) { detect_supports_ipv6? } + + private def self.detect_supports_ipv6? : Bool TCPServer.open("::1", 0) { return true } false rescue Socket::Error diff --git a/spec/support/time.cr b/spec/support/time.cr index d550a83af2c3..2b4738b5d71e 100644 --- a/spec/support/time.cr +++ b/spec/support/time.cr @@ -72,7 +72,9 @@ end # Enable the `SeTimeZonePrivilege` privilege before changing the system time # zone. This is necessary because the privilege is by default granted but # disabled for any new process. This only needs to be done once per run. - class_getter? time_zone_privilege_enabled : Bool do + class_getter?(time_zone_privilege_enabled : Bool) { detect_time_zone_privilege_enabled? } + + private def self.detect_time_zone_privilege_enabled? : Bool if LibC.LookupPrivilegeValueW(nil, SeTimeZonePrivilege, out time_zone_luid) == 0 raise RuntimeError.from_winerror("LookupPrivilegeValueW") end From 5728135995a13b58af29e81b7d0ee49b1eda237c Mon Sep 17 00:00:00 2001 From: Julien Portalier Date: Mon, 13 Jan 2025 18:20:41 +0100 Subject: [PATCH 6/7] crystal tool format --- src/crystal/once.cr | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/crystal/once.cr b/src/crystal/once.cr index ee52a7d9b54f..fd77fe04938c 100644 --- a/src/crystal/once.cr +++ b/src/crystal/once.cr @@ -21,8 +21,8 @@ # :nodoc: enum OnceState : Int8 Processing = -1 - Uninitialized = 0 - Initialized = 1 + Uninitialized = 0 + Initialized = 1 end @@once_mutex = uninitialized Mutex From cabbc9cf1067310cbcdb9ca1f510a7d669c2dc68 Mon Sep 17 00:00:00 2001 From: Julien Portalier Date: Mon, 13 Jan 2025 19:09:04 +0100 Subject: [PATCH 7/7] Fix: interpreter issues 1. it fails to translate symbol to the enum value 2. it doesn't call `__crystal_once_init` --- src/crystal/once.cr | 57 +++++++++++++++++++++++++++++---------------- 1 file changed, 37 insertions(+), 20 deletions(-) diff --git a/src/crystal/once.cr b/src/crystal/once.cr index fd77fe04938c..1a564866ad95 100644 --- a/src/crystal/once.cr +++ b/src/crystal/once.cr @@ -32,39 +32,43 @@ end # :nodoc: + # # Identical to `__crystal_once` but takes a block with possibly closured # data. Used by `class_[getter|property](declaration, &block)` for example. - @[AlwaysInline] - def self.once(flag : OnceState*, &block) : Nil + def self.once(flag : OnceState*, &) : Nil return if flag.value.initialized? - once(flag, block.pointer, block.closure_data) - Intrinsics.unreachable unless flag.value.initialized? + once_exec(flag) { yield } end # :nodoc: + # # Using @[NoInline] so LLVM optimizes for the hot path (var already # initialized). @[NoInline] def self.once(flag : OnceState*, initializer : Void*, closure_data : Void*) : Nil + once_exec(flag) { Proc(Nil).new(initializer, closure_data).call } + + # safety check, and allows to safely call `Intrinsics.unreachable` in + # `__crystal_once` + unless flag.value.initialized? + System.print_error "BUG: failed to initialize constant or class variable\n" + LibC._exit(1) + end + end + + private def self.once_exec(flag, &) @@once_mutex.synchronize do case flag.value in .initialized? return in .uninitialized? - flag.value = :processing - Proc(Nil).new(initializer, closure_data).call - flag.value = :initialized + flag.value = OnceState::Processing + yield + flag.value = OnceState::Initialized in .processing? raise "Recursion while initializing class variables and/or constants" end end - - # safety check, and allows to safely call `Intrinsics.unreachable` in - # `__crystal_once` - unless flag.value.initialized? - System.print_error "BUG: failed to initialize constant or class variable\n" - LibC._exit(1) - end end end @@ -101,10 +105,17 @@ @mutex = Mutex.new(:reentrant) @rec = [] of Bool* + def once(flag : Bool*, &) + return if flag.value + once_exec(flag) { yield } + end + @[NoInline] def once(flag : Bool*, initializer : Void*, closure_data : Void*) - return if flag.value + once_exec(flag) { Proc(Nil).new(initializer, closure_data).call } + end + private def once_exec(flag, &) @mutex.synchronize do return if flag.value @@ -113,7 +124,7 @@ end @rec << flag - Proc(Nil).new(initializer, closure_data).call + yield flag.value = true @rec.pop @@ -128,11 +139,9 @@ end # :nodoc: - @[AlwaysInline] - def self.once(flag : Bool*, &block) : Nil + def self.once(flag : Bool*, &) : Nil return if flag.value - @@once_state.once(flag, block.pointer, block.closure_data) - Intrinsics.unreachable unless flag.value + @@once_state.once(flag) { yield } end end @@ -151,3 +160,11 @@ Intrinsics.unreachable unless flag.value end {% end %} + +{% if flag?(:interpreted) %} + # make sure to initialize the mutex so we can use Crystal.once in the + # class_[getter|property]? macros; the compiler does the call by itself, but + # the interpreter doesn't (it doesn't use __crystal_once to protect the + # initialization of constants and class vars). + __crystal_once_init +{% end %}