Is the new gem thread safe?

[homanchou]

If we are using a sidekiq with multiple threads and do

Bigcommerce.configure do |config|
  config.store_hash = ENV['BC_STORE_HASH']
  config.client_id = ENV['BC_CLIENT_ID']
  config.access_token = ENV['BC_ACCESS_TOKEN']
end

# List orders
@orders = Bigcommerce::Order.all

in each worker, and they are executing at the same time, will Bigcommerce be isolated to the correct client in each worker?

[pedelman]

@homanchou the current version is not thread safe. I would love to support this, thanks for the suggestion!

I think parallel rspec tests are on the roadmap which should make it much easier to test this feature out.

[gregory]

Hey @homanchou thanks for reporting this.
Being thread safety is definitely on our TODO, but our focus right now is more on adding features/cleaning some stuffs.

We are really open to contributions though so if this is a priority for you, feel free to open PR!
thanks man

[jamespeerless]

Any update on this? Is @hasclass fork working?

[rounders]

I'm also interested in thread safety for this.

[strukturedkaos]

@gregory - Have there been any updates on thread safety?

[pedelman]

I am going to be taking a look over the two proposed implementations and taking a fresh look at the problem -- hope to have some proposals for discussion in the next few days.

[pedelman]

I have embarked on adding thread safety based on the implementation presented by @gregory. AFAIK this is NOT a breaking change and adds a new method to query resources by allow one to pass a new faraday connection into the resource.

Ex.

# Thread safe
conn = Bigcommerce::Connection.build(
  Bigcommerce::Config.new(
    store_hash: ENV['BC_STORE_HASH'], 
    client_id: ENV['BC_CLIENT_ID'], 
    access_token: ENV['BC_ACCESS_TOKEN']
  )
)

Bigcommerce::System.time(connenction: conn)

# Not thread safe
Bigcommerce.configure do |config|
  config.store_hash = ENV['BC_STORE_HASH']
  config.client_id = ENV['BC_CLIENT_ID']
  config.access_token = ENV['BC_ACCESS_TOKEN']
end

Bigcommerce::System.time

I have a WIP PR #123, if people could help test and give some review, that would be great. Cheers!

cc @gregory @homanchou @hasclass

[pedelman]

Here is the hacked together script I used to test the thread safety of the code between two stores. The threading implementation is using sucker_punch which in turn uses ruby-concurrent.

require 'bigcommerce'
require 'sucker_punch'

SuckerPunch.shutdown_timeout = 120

MAPPING = {
  'store_hash'   => 'email',
  'store_hash2'  => 'email2'
}.freeze

class UnsafeStoreInfoJob
  include SuckerPunch::Job

  def perform(store_hash, access_token)
    Bigcommerce.configure do |config|
      config.store_hash = store_hash
      config.access_token = access_token
      config.client_id = ENV['BC_CLIENT_ID']
    end

    sleep 0.1
    puts "UNSAFE #{MAPPING[store_hash] == Bigcommerce::StoreInfo.info.admin_email}"
  end
end

class SafeStoreInfoJob
  include SuckerPunch::Job

  def perform(store_hash, access_token)
    conn = Bigcommerce::Connection.build(
      Bigcommerce::Config.new(
        store_hash: store_hash,
        client_id: ENV['BC_CLIENT_ID'],
        access_token: access_token
      )
    )

    sleep 0.1
    puts "SAFE   #{MAPPING[store_hash] == Bigcommerce::StoreInfo.info(connection: conn).admin_email}"
  end
end

100.times do
  UnsafeStoreInfoJob.perform_async('store_hash', 'REDACTED')
  UnsafeStoreInfoJob.perform_async('store_hash2', 'REDACTED')

  SafeStoreInfoJob.perform_async('store_hash', 'REDACTED')
  SafeStoreInfoJob.perform_async('store_hash2', 'REDACTED')

  sleep 1
end

This (when using valid values) gives me an output like:

SAFE   true
UNSAFE true
SAFE   true
UNSAFE false
UNSAFE false
SAFE   true
SAFE   true
UNSAFE true
UNSAFE true
SAFE   true
UNSAFE false
SAFE   true
UNSAFE false
SAFE   true
UNSAFE true
SAFE   true
UNSAFE false
SAFE   true
UNSAFE true
SAFE   true
UNSAFE true
SAFE   true
...

I have run this quite a few times and have yet to see SAFE come back false -- indicating that the faraday connection is being reused between threads.

With this, I have confidence in saying if you use the connection param as proposed in #123 and by @gregory, you can ensure thread safety.