Clear the cache at the beginning of middleware (paranoid)

[stokarenko]

We saw recently that controller is trying to render the objects from yet another tenancy, it's just like showing the account data of random user within My profile page - serious thing you know )

Fortunately that happened for us in test suite - the cache of batch loader was leaking from the places out from true controller context, such as :controller and :view unit tests. That was not a production issue but still, it's much calmer to see the cache erased before the controller evaluation - just in case, the price of possible leak is high enough )

In production env the leak can happen still from initializer for instance.

[coveralls]

Coverage remained the same at 100.0% when pulling ce38992 on stokarenko:paranoid-cache into 94ffd9d on exAspArk:master.

[ghost]

This PR has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

[exAspArk]

Hey @stokarenko,

Thanks for opening the PR! Do you mind sharing more context on how it could in theory happen in production? My initial assumption is that:

• In production environment, the ensure block with BatchLoader::Executor.clear_current will always be executed. If it's not executed, then it means that something has crashed or aborted. E.g. the thread has died and it won't leak the state
• In test environment, there are chances when the middleware is not being used. E.g. for model tests when rack is not being used or not injected.

That's why the gem uses these lines in test environment:

batch-loader/spec/spec_helper.rb

Lines 35 to 37 in 4b7596a

	config.after do
	BatchLoader::Executor.clear_current
	end

It might be worth mentioning about clearing the state in test environment in the readme :)

[stokarenko]

Hey @exAspArk,

Do you mind sharing more context on how it could in theory happen in production?

The only thing from the top of my head - lets imagine that application uses the batch loader during the boot phase, within initializer or, say, as a result of eager loading of class like

class User < AR::Base
  ADMIN = self.batch_load_something.admins.first
end

This way the batch loader cache can leak to the first controller's action dispatched next to startup.

Anyway the question as always about "when to wash the beer glasses - after the party or before the next one?"
Typically we don't like to look at the dirty dishes and just pushing all the things into the dishwasher right after the party.

But we still can wipe the dust before the next action )

[exAspArk]

Thank you for your comment. I like your 🍻 analogy :)

Yeah, that's an interesting point. Basically, there are cases when people might combine using "out-of-band" batching with batching during regular request/response lifecycles in the same thread (e.g. single-threaded Sinatra)?

I was thinking about potentially removing BatchLoader::Executor.clear_current with the ensure block then. Not that it's an expensive operation, just to keep the code simple. I'm curious if there still could be some edge cases with BatchLoader being used after processing an HTTP request before processing another HTTP request 🙈 What do you think?

[stokarenko]

@exAspArk

From the paranoid point of view - let it be cleared both before and after the request )
It costs noting basically, and brings the feeling that we did everything we can for security )

[ghost]

This PR has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.