bknoles
commented
3 years ago Thank you @caifara ! You are correct on the problem, and I really like the IntertiaRailsRequest refactor. Even just creating the response method makes the code cleaner IMO.
I spent some time looking into middleware thread safety after seeing this, and for anyone reading this later, a bit more detail on the root issue is (by my understanding):
- Middlewares are instantiated once per process
- Threads are created within a process and share the same already-instantiated middleware classes
- Therefore, any instance variables created on a middleware class will be shared across all threads within the process
- InertiaRails was setting an
@envinstance variable on the middleware class on every request. This led to a single@envbeing used by every concurrently running request within a process. - Generally speaking, mutating instance variables on a middleware class within the
call(env)method makes the middleware not thread safe. (There are workarounds, but it's a good rule of thumb)
Resources I found helpful: https://stackoverflow.com/questions/23028226/rack-middleware-and-thread-safety https://crypt.codemancers.com/posts/2018-06-07-frozen-middleware-with-rack-freeze/ https://tenderlovemaking.com/2012/06/18/removing-config-threadsafe.html
caifara
Issue #58 happens when a delete request (or similar) and a get request are processed in different threads concurrently.
In the middleware @env isn't thread safe as it is an instance variable of the application (not scoped within the thread). That implicates that a new get request following a delete request on another thread would override @env during the delete request cycle thus not returning 303 but the Rails default 302.
I have two commits in this pr. The first one solves the problem by simply handing the env variable to every method depending on it. The second one uses a new class to be able to use the @env abstraction. In my opinion that approach is way cleaner and won't encourage other developers to reintroduce instance variables in the middleware itself.