Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Extend threadsafety to all ActiveResource::Base class attributes #120

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
4 participants
Contributor

peterjm commented Feb 21, 2014

Problem 1: Thread-safety

ActiveResource::Base has class attributes which makes it impossible to use in a threaded environment. The values for site, user, password and connection can all collide when using multiple threads.

99cbab7 started to add some thread safety to ActiveResource, by storing the headers in a thread local variable.

Solution

Extend the same solution from headers to the other class attributes: site, user, password and connection.

Problem 2: Inheriting values of thread-safe attributes between threads

With the current threadsafe implementation of headers, a new thread will also have a blank value for headers. This could lead to unexpected behaviour. For example, a typical Rails app might configure headers in an initializer, then use Sidekiq to process jobs. However, none of the Sidekiq jobs would inherit the initial configuration.

ActiveResource::Base.headers = {'User-Agent' => 'my-app'}
Thread.new do
  ActiveResource::Base.headers
  # => nil
end.join

Solution

Make new threads default their values to the main thread.

ActiveResource::Base.headers = {'User-Agent' => 'my-app'}
Thread.new do
  ActiveResource::Base.headers['Custom-Header'] = '1234'
  ActiveResource::Base.headers
  # => {'User-Agent' => 'my-app', 'Custom-Header' => '1234'}
end.join
ActiveResource::Base.headers = {'User-Agent' => 'my-app'}
# => {'User-Agent' => 'my-app'}
@peterjm peterjm Extend threadsafety to ActiveResource::Base.
Currently, `ActiveResource::Base.headers` is stored in a threadlocal variable. This commit extends
the same solution to other class attributes: `headers`, `site`, `user`, `password`, and
`connection`.
e9dc76b
Member

arthurnn commented Feb 21, 2014

Should we use PerThreadRegistry here instead?

@guilleiguaran thoughts?

Contributor

peterjm commented Feb 22, 2014

We might be able to use PerThreadRegistry here; but I think it would require creating a Registry-type class to store the class attributes. We wouldn't want to instantiate ActiveResource::Base objects to store the values.

Another advantage to my approach is that I made threads default their value to the value of the main thread, which I think will provide less unexpected behaviour. I've updated the PR description to point this out more clearly.

This was referenced Oct 20, 2014

Shouldn't headers and the other mentioned attributes be instance rather than thread-safe class variables?

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