Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensuring class_getter runs exactly once under concurrent access #14905

Open
HertzDevil opened this issue Aug 14, 2024 · 9 comments · May be fixed by #15387
Open

Ensuring class_getter runs exactly once under concurrent access #14905

HertzDevil opened this issue Aug 14, 2024 · 9 comments · May be fixed by #15387

Comments

@HertzDevil
Copy link
Contributor

A class_getter with a block usually runs exactly once:

module Foo
  class_getter(x : Int32) { 1 }

  # same as:
  def self.x : Int32
    if (x = @@x).nil?
      @@x = 1
    else
      x
    end
  end
end

However, if the current fiber is ever suspended, the block might be run multiple times:

module Foo
  class_getter x : Int32 do
    puts "Foo.x" # this runs 5 times
    Fiber.yield
    1
  end
end

ch = Channel(Int32).new
5.times { spawn { ch.send Foo.x } }
5.times { ch.receive }

And if -Dpreview_mt is in effect, the block also gets run more than once, even without the Fiber.yield.

Often, Foo.x represents some kind of cached object that might be expensive to compute, like #14891 and the Unicode data tables; if we also disregard the block's return value, then this would include the various interrupt handlers guarded by Atomic::Flags too. There should be an easier way in the standard library to ensure this block is really run exactly once, regardless of the degree of concurrent access.

Constants are one alternative, but whether they run at program startup or on first access is rather opaque, and also they are slightly broken, such as in #13054.

This could apply to class_property as well, and less likely to the instance variants getter and property.

@HertzDevil HertzDevil changed the title Class getters with blocks that run exactly once Ensuring class_getter runs exactly once under concurrent access Aug 14, 2024
@straight-shoota
Copy link
Member

Perhaps we can jerry rig an atomic compare and set on the type id? Then we'll just need an otherwise unused type to signal that someone else is already in the process of calculating.

@HertzDevil
Copy link
Contributor Author

Wouldn't that only work for mixed unions?

@straight-shoota
Copy link
Member

straight-shoota commented Aug 14, 2024

I suppose we could force a mixed union using an appropriate type to signal work in progress?
The value of the class variable would effectively be typeof({{ yield }}) | WorkInProgress | Nil.
If the type of the block is a reference type, WorkInProgress could be a struct type and vice versa. That would mean two different signal types and select the appropriate one.

@straight-shoota
Copy link
Member

This is related to #15085. Perhaps we could use the same mechanism for both (for example a spin-lock on the type id).

@crysbot
Copy link
Collaborator

crysbot commented Oct 18, 2024

This issue has been mentioned on Crystal Forum. There might be relevant details there:

https://forum.crystal-lang.org/t/charting-the-route-to-multi-threading-support/7320/1

@ysbaddaden ysbaddaden moved this from Backlog to Todo in Multi-threading Jan 8, 2025
@ysbaddaden ysbaddaden self-assigned this Jan 8, 2025
@ysbaddaden
Copy link
Contributor

The simplest solution would be to replace the "lazy method" with an explicit assignment: thread safety would handled by __crystal_once and it would even check for recursion (win-win situation). Since {{yield}} is injected as begin; ...; end it works perfectly...

With the exception that @@cvar = value is considered user code, and will be inlined right into __crystal_main and be eagerly loaded when the program starts. Damn, that broke the lazyness.

Note: apparently codegen will always declare & initialize all class vars with an initializer (default value); while LLVM will optimize them away if the program never accesses them, dev builds (i.e. -O0) will always initialize all class vars (be they accessed or not).

@ysbaddaden
Copy link
Contributor

I tried to reused __crystal_once but it didn't go well: the Mutex for example is only used when preview_mt is enabled (fixable) but the fun only takes a proc pointer with a closure pointer, and that won't work, and I get invalid memory accesses with the example above.

I'll have to write something custom, that will take and call an actual Proc and will need a Mutex.

@ysbaddaden
Copy link
Contributor

I was eventually able to reuse Crystal.once by adding a yielding version.

Using the captured block would allow for a better optimization: always inline the quick check and never inline the actual initialization.

But capturing the block would be breaking change (can't return nor break anymore) even though the cases in std lib/spec where we used return didn't work as expected: the value was returned but the class var was never set.

We might want to introduce an opt-in flag.

@ysbaddaden ysbaddaden moved this from In Progress to Review in Multi-threading Jan 21, 2025
@straight-shoota
Copy link
Member

Capturing the block would be nice 👍
The breaking change is intricate though. If any initializer code uses return or break, it probably does so without realizing or intending the implication (we even have a few cases in stdlib).
So one might even argue that changing to a captured block might help discover bugs in lazy initializers 🤷 It can be a good thing to let that code break that behaves unexpectedly/unintendedly.

@ysbaddaden ysbaddaden moved this from Review to In Progress in Multi-threading Jan 21, 2025
straight-shoota pushed a commit that referenced this issue Jan 25, 2025
….init_runtime` (#15369)

Adds `Crystal.init_runtime` which calls `Thread.init`, `Fiber.init` to explicitly initialize the class variables defined on `Thread` and `Fiber`.

In order to implement #14905 we'll need the ability to access `Fiber.current` which depends on `Thread.current` but if these accessors use the class getter/property macros then we'll enter an infinite recursion.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment