Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make ProxyChain thread-safe #7

Merged
merged 1 commit into from
Feb 6, 2016
Merged

Conversation

saidie
Copy link

@saidie saidie commented Jun 20, 2015

This PR fixes the issue #5 by using thread local variable in ProxyChain.
Sorry in advance if the name of the thread local variable is not preferable.

@saidie
Copy link
Author

saidie commented Jun 20, 2015

I have a test case which fails without this patch but I'm hesitating to include it since it uses ugly sleep:

require "spec_helper"

describe "ProxyChain thread-safety" do

  class Proxy < Arproxy::Base
    def execute(sql, name)
      sleep 0.01
      { connection: self.proxy_chain.connection }
    end
  end

  module ::ActiveRecord
    module ConnectionAdapters
      class DummyAdapter
        def execute(sql, name = nil)
        end
      end
    end
  end

  let(:connection) { ::ActiveRecord::ConnectionAdapters::DummyAdapter.new }

  before do
    allow(Arproxy).to receive(:logger) { Logger.new('/dev/null') }
    Arproxy.clear_configuration
    Arproxy.configure do |config|
      config.adapter = "dummy"
      config.use Proxy
    end
    Arproxy.enable!
  end

  after(:each) do
    Arproxy.disable!
  end

  context "with two threads" do
    let!(:thr1) { Thread.new { connection.dup.execute '', '' } }
    let!(:thr2) { Thread.new { connection.dup.execute '', '' } }

    it { expect(thr1.value[:connection]).not_to eq(thr2.value[:connection]) }
  end

end

@mirakui mirakui merged commit d84c8dd into cookpad:master Feb 6, 2016
mirakui pushed a commit that referenced this pull request Feb 6, 2016
@mirakui
Copy link
Collaborator

mirakui commented Feb 6, 2016

Thank you for this PR! Released as v0.2.1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants