search

steveklabnik / request_store

Per-request global storage for Rack.
https://github.com/steveklabnik/request_store
MIT License
1.46k stars 88 forks source link

Should RequestStore use "true" thread locals? ie `Thread.current.thread_variable_get` #86

Open palasha opened 2 years ago

palasha commented 2 years ago

I recently learned about the difference between Fiber local Thread.current[:foo]= ... and real Thread locals Thread.current.thread_variable_set(:foo, ...). The latter are preserved across Fibers within a thread.

I'm writing a library that depends on this distinction, where it would also be useful to have the same functionality as RequestStore, but I can't currently use it because RequestStore relies on Thread.current[].

Has this library ever considered implementing its behavior using real thread locals? Given that even a multi-threaded rails server only processes 1 request per thread at a time, it seems like that should work similarly well.

I did a quick github search and didnt find any issues / prs / commits with these keywords so figured I would ask the question here.

steveklabnik commented 2 years ago

Fibers technically existed but were used by no-one when I wrote this library. I haven't really used Ruby much since they became popular.

I think the key question would be, are there any popular webservers that do a "request per fiber" style. Because if this data was persisted across fibers, you'd reintroduce the same problem, just one conceptual layer down.

steveklabnik commented 2 months ago

Well, we ran into https://github.com/steveklabnik/request_store/issues/96 and similar so while we tried to support fibers that way, we are now back to what this was when this first post was made.

I don't know about the distinction between these two at all, so it's hard for me to comment.

marcotc commented 2 months ago

We are having the same discussion at ddtrace (https://github.com/DataDog/dd-trace-rb/issues/1389, https://github.com/rmosolgo/graphql-ruby/issues/3366, https://github.com/rmosolgo/graphql-ruby/pull/3461), and I'm leaning towards moving the storage away from Fiber-local and towards Thread-local.

It's super hard to predict the full impact on users' application, but I think it's worth investigation. I'm thinking of providing a opt-in flag to change the behaviour. In an ideal world, we would have perfect, real-world testing and would either make this change once and for all or leave it as is, based on user feedback, but I think it's super hard for us to actually do that confidently.