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

Why does current_store_id default to nil? #11

Open
fsateler opened this issue Apr 19, 2021 · 1 comment
Open

Why does current_store_id default to nil? #11

fsateler opened this issue Apr 19, 2021 · 1 comment

Comments

@fsateler
Copy link

Hi,

I was bit recently by a bug because a thread (created by a library) did not properly initialize current_store_id. I added a monkey patch to add that, and all is working well now.

However, this got me thinking: why is it possible for current_store_id to ever return nil? I believe the safer default is to always set to SecureRandom.uuid if uninitialized:

def current_store_id
  Thread.current.fetch(REQUEST_STORE_ID) { SecureRandom.uuid }
end

The current default basically makes all the spawned threads share a store between them, but separate from the main thread.

It may be an even safer default to raise an error when the request store is unset. It probably means a thread was spawned without proper setup/teardown

@ElMassimo
Copy link
Owner

ElMassimo commented Apr 21, 2021

Hi Felipe, that's a good observation.

  • The current behavior is definitely not desirable
  • Using SecureRandom.uuid might cover up problems when threads are not properly initialized
  • A potential problem with raising an error, is that it would stop working in non-request lifecycles (for example, in a console)

A possible solution would be to have the main thread use true unless specified, and raise an error for any child threads that are not explicitly initialized.

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants