Integrate request_store to help with Threading issues #340

Merged
merged 1 commit into from Mar 3, 2014

4 participants

@steveklabnik

Hey there! I ran into issues using Thread.current while working with Draper, and apparently Authlogic has them too. So I wrote a gem to handle it, and I figured you may want to use it too. Here's the Draper discussion: drapergem/draper#390

Fixes #335.

The Issue

If you use Thread.current to store state, and you use one of those fancy threaded servers like Puma or Thin, then what's in Thread.current doesn't get cleared per-request like it would if you were using Webrick or whatever. This causes subtle bugs.

The Fix

Basically, I wrote a gem that gives you 'request local' storage. It quacks like a Hash, just like Thread.current, but a middleware ensures that you get a clean slate every request.

I ran your tests on 1.9.3 and they were passing, but the gem works with basically every modern Ruby ever (and some not-so-modern ones), so no issues there.

Thanks! ❤️ ❤️ ❤️

@tiegz
Collaborator

👍 👍 👍 that was fast, thanks @steveklabnik!!

@binarylogic
Owner

I like this, want to look at it more. Not a big fan of creating a new dependency, but not sure I have a choice here.

@steveklabnik
@nathany

This seems important. Is anyone already using Authlogic with Puma or another thread web-server? Have changes in Puma or Ruby 2 or 2.1 made this unnecessary?

@binarylogic
Owner

I'm on board, if someone wants to solve the merge confllcits I'll pull in. Otherwise I'll have to do it this weekend some time.

@nathany

I've noticed that Rails itself uses Thread.current, so I wonder if this is still necessary?

@tiegz
Collaborator

@nathany yeah, Thread.current is the problem itself. Authlogic sets the controller variable, but doesn't unset it after the request. I ran into an issue on a non-threaded server where activate_authlogic was set after another filter checked the user, so the filter grabbed the previous request's user (luckily it was an innocuous filter). RequestStore will ensure each request gets its own thread-local vars.

@steveklabnik

I can solve the merge conflicts soon if this patch is wanted.

And yes, @tiegz, exactly.

@steveklabnik

I've updated the PR to fix the merge conflicts.

@binarylogic
Owner

sweet thanks

@binarylogic binarylogic merged commit 5bd696a into binarylogic:master Mar 3, 2014
@steveklabnik

😍 👍 🎊

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment