steveklabnik / request_store

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

RequestStore's use of Fiber and whether storage is duplicated across Fibers. #98

Closed professor closed 4 months ago

professor commented 4 months ago

I'm debugging an issue in PaperTrail and how it uses RequestStore.

I noticed that one of their tests failed with the switch from Ruby 3.1.5 to 3.2.0:

ruby version rails version status RequestStore.scope
3.0.1 7.1 green
3.1.3 7.1 green
3.1.5 7.1 green Thread
3.2.0 7.1 red Fiber
3.2.4 7.1 red
3.3.1 7.1 red

In the test they assert that a new thread would get a new RequestStore:

Thread.new { expect(described_class.whodunnit).to be_nil }.join
Thread.new { expect(described_class.enabled_for_model?(Widget)).to eq true }.join

I'm able to update the test for RequestStore and Fiber like so:

Fiber.new(storage: nil) { expect(described_class.whodunnit).to be_nil }.resume
Fiber.new(storage: nil) { expect(described_class.enabled_for_model?(Widget)).to eq true }.resume

Notice that when a new Fiber is created, by default it "inherits a copy of the storage from the current fiber."

I realize that RequestStore does not specify how Fibers are created, and this made me wonder if there's a potential bug in using RequestStore since it may be hard to see how new Fibers are created in puma or thin. (Currently puma and thin use Thread.new)

mattbrictson commented 4 months ago

There is also a discussion in https://github.com/steveklabnik/request_store/issues/96#issuecomment-1974215624 that I think concerns the same underlying issue. Note that request_store 1.6.0 switched from thread local storage to fiber storage, which is what triggered the problem in my case.

professor commented 4 months ago

Thank you @mattbrictson -- let's ammend the issue title in #96 -- I had assumed it wasn't related.