search

railslove / rack-tracker

Tracking made easy: Don’t fool around with adding tracking and analytics partials to your app and concentrate on the things that matter.
https://www.railslove.com/open-source
MIT License
647 stars 121 forks source link

Make middleware thread safe #150

Closed kspe closed 4 years ago

kspe commented 4 years ago

We've noticed that in high traffic threaded environment (Puma) this middleware shares instance variables memory between threads and occasionally swaps ivars content.

In a worst-case scenario observed for a request from user A the response that was coming back was from a request made by a user B. If Set-Cookie header was present user A was immediately logged out and logged in as a user B (completely confusing users).

We've observed also a number of other requests/responses swaps which in general were hard to debug e.g. API request calls returning non-API responses.

This PR does not remove using of ivars (@status, @headers, @body) but rather duplicates the middleware calls. We are using forked repo for a few days now and we do not observe the issue anymore.

For more issues similar to this one you can check here , here and here in SO thread.

If this won't be planned for merging after all, maybe at least will save some time for others debugging similar symptoms.

bumi commented 4 years ago

oh, thanks a lot for the PR! why are those instance variables in the first place, :thinking: I'd even try to get rid of those instance variables. @DonSchado could you take a look? maybe merge this PR, release a patch and then these instance vars should be refactored?

DonSchado commented 4 years ago

Thanks for raising this issue @kspe very appreciated! If we inline the html? method, we could remove all instance vars.

But very interesting, I also don't know if this pattern was just copied over from the former project, but now I believe the issue makes sense in the way rack initializes middleware.

Is #dup even necessary, when we refactor/remove the internal state?

kspe commented 4 years ago

Is #dup even necessary, when we refactor/remove the internal state?

I think the main issue is that middleware maintains a mutable state in ivars. Without those, it should be safe to remove dup. The worst thing is that it is hard to test in specs, easier with real applications run locally and bombarded with many requests at a high rate.

bumi commented 4 years ago

Thank you! <3