salsify / goldiloader

Just the right amount of Rails eager loading
MIT License
1.61k stars 53 forks source link

Do not persist globally enabled in threads #110

Closed bdurand closed 3 years ago

bdurand commented 3 years ago

This fixes a subtle bug that can cause unpredictable behavior if globally_enabled is changed (i.e. during app initialization).

With the current code, whenever enabled or disabled is called with a block, the previous value is restored to the thread local variable. However, if the previous value was not set, then the value that is stored is the value of globally_enabled.

Consider this contrived example:

Goldiloader.globally_enabled = true

ThreadPool.run do
  Goldiloader.enabled { do_thing } # Thread[: goldiloader_enabled] now set to true in this thread
end

Goldiloader.globally_enabled = false

ThreadPool.run do
  Goldiloader.enabled { do_thing } # Thread[: goldiloader_enabled] now set to false in this thread
end

Now, assuming we had two different threads running in the thread pool, we have two different states for Goldiloader.enabled. I would expect that changing the global setting would change the default state for all threads.

jturkel commented 3 years ago

Released in 4.1.1.