Use of Thread.current and ViewContext #390

Closed
elado opened this Issue Dec 14, 2012 · 16 comments

Projects

None yet

6 participants

@elado

I think this is pretty serious bug, where view context is kept globally and not per-request:

On a server like thin or unicorn, the Thread.current hash is not being cleared between requests. It means that this code:

before = Thread.current[:x]
Thread.current[:x] = 1
render text: [ before, Thread.current[:x]  ].inspect

in a controller, when executed twice (even from different sessions), will print nil, 1, and then 1, 1.

It means that Thread.current[:current_view_context] ||= build_view_context will keep previous value.
And h.current_user, on first hit, will return the current user, and on a different session will return the previous user because the Thread.current[:current_view_context] contains the old context.

I think that the solution is to clear the Thread.current[:current_view_context] in a Middleware, before the request, so it gets fetched on every request.

See more discussion and conclusions here:
http://stackoverflow.com/questions/13829257/rails-how-to-handle-thread-current-data-under-a-single-threaded-server-like-thi

Thoughts?

@steveklabnik
drapergem member

I've always been slightly weirded out by this code, so yes, I am totally open to changing it.

@steveklabnik
drapergem member

Related: rails/rails#8517

@tiegz

👍 on a solution for this. I've run into an issue with Authlogic in the past regarding this, since it sets Thread.current[:controller] in a before_filter but never unsets it. So when the filter chain halts before Authlogic's you're stuck with the last request's controller: binarylogic/authlogic#335

@mperham

Yes, either use begin/ensure to clear it or reset it at the start of request.

@steveklabnik
drapergem member

@mperham thanks for chiming in. ❤️.

By begin/ensure you mean in some sort of middleware? Would that be appropriate to push up into Rails proper, especially as we're making Rails 4 be threadsafe by default?

@mperham
@glebm

The way to got about it without breaking stuff is:

  • Clear that variable before request. Clearing it after request is difficult to do cleanly, because exceptions might result in after filter never being triggered
  • Clearing all the variables is a bit dangerous, because Thread.current may be used for keeping permanent per-thread data, such as stats, profiling information, separate log outputs per threads, etc... For a typical Rails application this is probably not an issue, but clearing Thread.current will cause issues when trying to debug local memory leaks and such.
@steveklabnik steveklabnik added a commit that closed this issue Dec 17, 2012
@steveklabnik steveklabnik Integrate RequestStore.
Fixes #390
fde1cde
@steveklabnik
drapergem member

@elado I've pushed a new commit for Draper that relies on my new gem, RequestStore, and I believe that it will fix this issue. Please try out Draper HEAD as soon as you're able, and let me know if this fixes it.

Thank you!

@elado

@steveklabnik Funny, I wrote the same thing and just didn't push it :) Thanks, I'll check it out soon.

@steveklabnik
drapergem member
@elado

You shouldn't be! Thanks again.

@elado

Worth mentioning: plataformatec/devise#2182

@steveklabnik
drapergem member

I'll investigate and coordinate with @josevalim, thank you.

@josevalim

The current RequestStore should not cause any issues with Devise, since it is cleaned at the beginning of the request and not inside ensure.

@elado

Since you don't clear the value after app.call it's not going to be a problem.

The common approach of middlewares and Thread.current i've seen so far was something like

def call(env)
  old, Thread.current[:key] = Thread.current[:key], {}
  @app.call(env)
ensure
  Thread.current[:key] = old
end

Not sure how much the reassigning of the old value is necessary, but without it it works because the value is still available after Warden's @app.call.

@steveklabnik
drapergem member

:metal:

Yeah, I figured the least invasive thing was to only touch it at the start of the request.

@steveklabnik steveklabnik added a commit to steveklabnik/authlogic that referenced this issue Mar 2, 2014
@steveklabnik steveklabnik Integrate request_store to solve threading issues.
Fixes #335.

See also drapergem/draper#390
9fd44b9
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment