When a request finishes in one thread, RequestLocals is cleared for all other threads

[ndollar]

https://github.com/ElMassimo/request_store_rails/blob/master/lib/request_store_rails/railtie.rb#L15

I'm running into an issue where I have multiple concurrent requests and whenever the first request finishes, RequestLocals.clear_all! is called, which clears the store for every other requests.

I'm running puma

Scenario

time=0 | request_id=1 | Start Request, create store[1] time=1 | request_id=2 | Start Request, create store[2] time=2 | request_id=1 | Complete Request -> RequestLocals.clear_all! time=3 | request_id=2 | Exception because store[2] is cleared

Is RequestLocals.clear_all! supposed to be called after each request? Or can we call RequestLocals.clear! instead?

Thanks! 😄

[ElMassimo]

Hey there, for post-request cleanup you'll want to call RequestLocals.clear! instead, as in the Sidekiq middleware example.

RequestLocals.clear_all! will effectively clear all stores, for all threads, and is only intended for usage in development (it's used every time your local Rails server is reloaded).

Hope that clarifies the difference between the two, cheers!

[ndollar]

@ElMassimo sorry I didn't specify it in the ticket. This issue is only happening for me in development because of ActionDispatch::Reloader.to_cleanup(&clear_all) https://github.com/ElMassimo/request_store_rails/blob/master/lib/request_store_rails/railtie.rb#L15

(it's used every time your local Rails server is reloaded).

ActionDispatch::Reloader.to_cleanup is called every time a request completes. https://github.com/rails/rails/blob/4-2-stable/actionpack/lib/action_dispatch/middleware/reloader.rb#L46

So in development when one request completes, it clears the store for the other requests.

Thanks!

[ElMassimo]

Not sure how dev reload works in multi-threaded servers (does it wait for a request to finish before reloading the code?).

Not clearing the store on reload could potentially keep a reference to instances of classes that has been removed from the module tree (replaced with new classes after reload).

You could try monkeypatching clear_all! to behave differently in your use case, but it might create other problems.

Happy to take a patch if you figure out how to deal with the problem in multi-threaded development servers in a consistent way.

[ndollar]

Ok great! Agreed don't want to leak memory. I'll look into a solution. Thanks.

P.S.

Not sure how dev reload works in multi-threaded servers (does it wait for a request to finish before reloading the code?).

I edited my comment above too many times 😄 . Reposting here. ActionDispatch::Reloader.to_cleanup is called every time a request completes. https://github.com/rails/rails/blob/4-2-stable/actionpack/lib/action_dispatch/middleware/reloader.rb#L46

So in development when one request completes, it clears the store for the other requests.

[ndollar]

Hey @ElMassimo, I finally got some time to look back into this.

I think you may be able to get rid of the Railtie all together.

Not clearing the store on reload could potentially keep a reference to instances of classes that has been removed from the module tree (replaced with new classes after reload).

Every request clears its own store via the Middelware call to clear!, so code would somehow have to reload mid request in order to cause the problem you're referring to.

The Railtie->clear_all! runs whenever a Request Body is closed https://github.com/rails/rails/blob/4-2-stable/actionpack/lib/action_dispatch/middleware/reloader.rb#L9-L11

It basically duplicates the Middleware->clear!call and it wouldn't prevent the mid-request reload scenario. And in the mult-threaded case, it clears all other concurrent request stores, which is my issue.

Would you be open to removing the Railtie?

Thanks!

[ElMassimo]

We need the Railtie to clear the store in the console.

Happy to accept a pull request that limits clear_all! to the console, perhaps registering the reloader callback in the Railtie inside a console block instead.

[ndollar]

Makes sense! Will look into it.

That's a use case I didn't think about. How is RequestStoreRails supposed to behave in the console? Is there a concept of current_request_id?

[ElMassimo]

In the console RequestLocals.current_store_id is always nil unless explicitly set.

As a result, there would only be one store in the console, you can think of it as "a single request".