Merge pull request #95 from Shopify/threadsafe_headers

Threadsafe headers

.travis.yml

@@ -9,6 +9,7 @@ gemfile:
   - Gemfile_ar30
   - Gemfile_ar31
   - Gemfile_ar32
+  - Gemfile_ar40threadsafe
 
 install: bundle install
 
@@ -28,5 +29,5 @@ matrix:
 
 
 notifications:
-  flowdock: 
+  flowdock:
     secure: "RCgSxzMScnqK6bOwkv9sWSdieLBeJla8NcDtM/QmuFW8soTgV6qCTAPAGd4lpjg4vGTaM3DsdHU5GMDbgdWN5dE2Rf09ayFqiZg8OloPMQ63KIwLJyVcw3cVJO5i7smjIpsSjPjBkvAXHIOFcKdsnuYGS4YD8hjl+QrZ3ghi440="

Gemfile_ar40threadsafe

@@ -0,0 +1,5 @@
+source "https://rubygems.org"
+
+gemspec
+
+gem 'activeresource', :git => 'git://github.com/Shopify/activeresource', :ref => 'e9dc76b4aa'

README.rdoc

@@ -139,6 +139,22 @@ This package also includes the +shopify+ executable to make it easy to open up a
     shopify help
 
 
+== Threadsafety
+
+ActiveResource is inherently non-threadsafe, because class variables like `ActiveResource::Base.site` and
+`ActiveResource::Base.headers` are shared between threads. This can cause conflicts when using
+threaded libraries, like Sidekiq.
+
+We have a forked version of ActiveResource that stores these class variables in threadlocal
+variables. Using this forked version will allow ShopifyAPI to be used in a threaded environment.
+
+To enable threadsafety with ShopifyAPI, add the following to your Gemfile:
+
+```
+gem 'activeresource', :git => 'git://github.com/Shopify/activeresource', :ref => 'e9dc76b4aa'
+gem 'shopify_api', '>= 3.3.0'
+```
+
 == Using Development Version
 
 Download the source code and run:

lib/shopify_api/resources/base.rb

@@ -14,7 +14,7 @@ module ShopifyAPI
 
       same.send("to_#{self.class.format.extension}", options)
     end
-    
+
     def as_json(options = nil)
       root = options[:root] if options.try(:key?, :root)
       if include_root_in_json
@@ -26,13 +26,25 @@ module ShopifyAPI
     end
 
     class << self
-      def headers
-        if defined?(@headers)
-          @headers
-        elsif superclass != Object && superclass.headers
-          superclass.headers
-        else
-          @headers ||= {}
+      if ActiveResource::VERSION::MAJOR == 4 && ActiveResource::VERSION::PRE == 'threadsafe'
+        def headers
+          if _headers_defined?
+            _headers
+          elsif superclass != Object && superclass.headers
+            superclass.headers
+          else
+            _headers ||= {}
+          end
+        end
+      else
+        def headers
+          if defined?(@headers)
+            @headers
+          elsif superclass != Object && superclass.headers
+            superclass.headers
+          else
+            @headers ||= {}
+          end
         end
       end
 

test/base_test.rb

@@ -8,20 +8,24 @@ class BaseTest < Test::Unit::TestCase
     @session2 = ShopifyAPI::Session.new('shop2.myshopify.com', 'token2')
   end
 
+  def teardown
+    clear_header('X-Custom')
+  end
+
   test '#activate_session should set site and headers for given session' do
     ShopifyAPI::Base.activate_session @session1
 
     assert_nil ActiveResource::Base.site
     assert_equal 'https://shop1.myshopify.com/admin', ShopifyAPI::Base.site.to_s
     assert_equal 'https://shop1.myshopify.com/admin', ShopifyAPI::Shop.site.to_s
-    
+
     assert_nil ActiveResource::Base.headers['X-Shopify-Access-Token']
     assert_equal 'token1', ShopifyAPI::Base.headers['X-Shopify-Access-Token']
     assert_equal 'token1', ShopifyAPI::Shop.headers['X-Shopify-Access-Token']
   end
 
   test '#clear_session should clear site and headers from Base' do
-    ShopifyAPI::Base.activate_session @session1    
+    ShopifyAPI::Base.activate_session @session1
     ShopifyAPI::Base.clear_session
 
     assert_nil ActiveResource::Base.site
@@ -34,8 +38,8 @@ class BaseTest < Test::Unit::TestCase
   end
 
   test '#activate_session with one session, then clearing and activating with another session should send request to correct shop' do
-    ShopifyAPI::Base.activate_session @session1   
-    ShopifyAPI::Base.clear_session    
+    ShopifyAPI::Base.activate_session @session1
+    ShopifyAPI::Base.clear_session
     ShopifyAPI::Base.activate_session @session2
 
     assert_nil ActiveResource::Base.site
@@ -49,9 +53,54 @@ class BaseTest < Test::Unit::TestCase
 
   test "#delete should send custom headers with request" do
     ShopifyAPI::Base.activate_session @session1
-    ShopifyAPI::Base.expects(:headers).returns({'X-Custom' => 'abc'})
-    ShopifyAPI::Base.connection.expects(:delete).with('/admin/bases/1.json', {'X-Custom' => 'abc'})
+    ShopifyAPI::Base.headers['X-Custom'] = 'abc'
+    ShopifyAPI::Base.connection.expects(:delete).with('/admin/bases/1.json', has_entry('X-Custom', 'abc'))
     ShopifyAPI::Base.delete "1"
   end
 
+  test "#headers includes the User-Agent" do
+    assert_not_includes ActiveResource::Base.headers.keys, 'User-Agent'
+    assert_includes ShopifyAPI::Base.headers.keys, 'User-Agent'
+    thread = Thread.new do
+      assert_includes ShopifyAPI::Base.headers.keys, 'User-Agent'
+    end
+    thread.join
+  end
+
+  if ActiveResource::VERSION::MAJOR >= 4
+    test "#headers propagates changes to subclasses" do
+      ShopifyAPI::Base.headers['X-Custom'] = "the value"
+      assert_equal "the value", ShopifyAPI::Base.headers['X-Custom']
+      assert_equal "the value", ShopifyAPI::Product.headers['X-Custom']
+    end
+
+    test "#headers clears changes to subclasses" do
+      ShopifyAPI::Base.headers['X-Custom'] = "the value"
+      assert_equal "the value", ShopifyAPI::Product.headers['X-Custom']
+      ShopifyAPI::Base.headers['X-Custom'] = nil
+      assert_nil ShopifyAPI::Product.headers['X-Custom']
+    end
+  end
+
+  if ActiveResource::VERSION::MAJOR >= 4 && ActiveResource::VERSION::PRE == "threadsafe"
+    test "#headers set in the main thread affect spawned threads" do
+      ShopifyAPI::Base.headers['X-Custom'] = "the value"
+      Thread.new do
+        assert_equal "the value", ShopifyAPI::Base.headers['X-Custom']
+      end.join
+    end
+
+    test "#headers set in spawned threads do not affect the main thread" do
+      Thread.new do
+        ShopifyAPI::Base.headers['X-Custom'] = "the value"
+      end.join
+      assert_nil ShopifyAPI::Base.headers['X-Custom']
+    end
+  end
+
+  def clear_header(header)
+    [ActiveResource::Base, ShopifyAPI::Base, ShopifyAPI::Product].each do |klass|
+      klass.headers.delete(header)
+    end
+  end
 end