search

igorkasyanchuk / rails_performance

Monitor performance of you Rails applications (self-hosted and free)
https://www.railsjazz.com/
MIT License
957 stars 54 forks source link

RailsPerformance::Rails::Middleware maybe not thread safe #8

Closed jac241 closed 3 years ago

jac241 commented 3 years ago

Hello. Love this gem and have been using it on my rails app (AnkiAchievements.com) which is open source at github.com/jac241/spaced_rep_achievements . Unfortunately, I noticed an issue relating to thread safety and your middleware. Users were reporting becoming logged in as another user while using the app - i.e. the headers and responses for the two different requests were either being swapped or the first was being overwritten by the second. I looked back through the logs and saw that this would occur when two requests exactly overlapped. I use a common, threaded webserver (puma).

I think setting @status, @headers, @response = @app.call(env) and then returning those instance variables makes the middleware not thread safe and could be the source of my problems. I am not a puma expert, but I think it only instantiates the middlewares once and then shares the single instances with each thread, so setting/updating an instance variable could overwrite one set by a different thread.

jac241 commented 3 years ago

It looks like this guide suggests calling #.dup on the middleware instance before calling the real #call method, which would prevent a different thread from overwriting the instance variables.

https://ieftimov.com/post/writing-rails-middleware/

igorkasyanchuk commented 3 years ago

Hi @jac241 this is interesting can you make a PR with a fix? and try it on your site if it helps?

igorkasyanchuk commented 3 years ago

I see that this is many who do it, for example: https://github.com/sinatra/sinatra/blob/master/lib/sinatra/base.rb#L928-L931

so I've just released a new version