steveklabnik / request_store

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

Use fiber storage if available for #90

Closed ioquatix closed 1 year ago

ioquatix commented 1 year ago

RequestStore uses fiber-local storage, so it doesn’t get passed down into child fibers (or threads). When using the fiber scheduler, request state can be lost when creating child fibers. Fiber storage is inherited by child fibers, and so using it avoids the problem. In every other way, it behaves like fiber locals.

Similar to https://github.com/BMorearty/request_store-fibers in theory.

cc @BMorearty

BMorearty commented 1 year ago

Thank you, this is an improvement over my https://github.com/BMorearty/request_store-fibers gem, which really felt like a hack to work around the problem of using RequestStore with Falcon or anything that uses fibers.

The only caution with this change is that the behavior of RequestStore changes from thread-local to fiber-local when you upgrade your Ruby version. (I don't know which version introduced Fiber::[].) This PR should document this change in the README, since the README explicitly describes the implementation details.

ioquatix commented 1 year ago

Actually Thread.current[:request_store] is already fiber local.

BMorearty commented 1 year ago

@ioquatix understood, but that's just sloppy phrasing on my part. My point still stands: the README needs to be updated in this PR to reflect the change.

ioquatix commented 1 year ago

Yes, completely agree that we should update the documentation.

ioquatix commented 1 year ago

I checked the README.md and could not see any parts that need to change. Do you mind pointing out what you think should be updated?

BMorearty commented 1 year ago

@ioquatix You're correct, when I skimmed the README I saw a lot of references to Thread.current but didn't realize those are just examples of how you could implement a request store by yourself if you weren't using RequestStore.

So as far as I'm concerned this PR is good to go. 👍🏻

ioquatix commented 1 year ago

@steveklabnik Hello :)

steveklabnik commented 1 year ago

Thank you! This makes sense.

Seems like some tests are failing on new rubies though...

ioquatix commented 1 year ago

Ahh I didn't see that, let me check.

ioquatix commented 1 year ago

Can someone please press the button to run the tests.

steveklabnik commented 1 year ago

@ioquatix sorry about that! I figured the first time I pressed it, it would run it for you again every time.

Thanks! All green now.

mintuhouse commented 11 months ago

Does Fiber storage inheritance/reset work for nested hashes?


Fiber[:request_store][:test] = 1
f = Fiber.new do
  Fiber[:request_store][:test] = 2
end
f.resume
puts Fiber[:request_store][:test] #=> Returns 2 instead of 1

As a result, after this PR

RequestStore.store[:test] = 1
f = Fiber.new do
  RequestStore.store[:test] = 2
end
f.resume
puts RequestStore.store[:test] #=> Also returns 2

Is this the expected behaviour?

ioquatix commented 11 months ago

I think that's the expected behaviour. The store is only shallow copied.