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 bigcommerce threadsafe #104

Closed
wants to merge 2 commits into from
Closed

Conversation

hasclass
Copy link

I made a quick prototype on how to add threadsafety (Issue #98) without too much refactoring (which I think is crucial)

Implementation is using Thread.current. I haven't tested it in real-life, as my bigcommerce dev account isn't active yet.

Integration into rails would be like follows:

around_filter :configure_bigcommerce

protected

def configure_bigcommerce
  config = {store_hash: session[:store_hash], client_id: "..", access_token: ".."}
  Bigcommerce.configure_threadsafe(config) do
    yield
  end
end

if this approach is ok for you i would finish this and add tests.

@gregory
Copy link
Contributor

gregory commented Jan 19, 2016

@hasclass - i actually started on another refactoring for thread safety here, but haven't got time to continue on it.
What do you think about it

@hasclass
Copy link
Author

@gregory that looks like the hard but proper way of adding thread saftety.

@srushe
Copy link

srushe commented May 19, 2016

Has there been any progress on either approach? Or is threading support still expected to appear at some point?

@pedelman
Copy link
Contributor

@hasclass I actually rather like the idea of this PR, do you need any help to finish? I am happy to build off your current implementation and do some extensive testing on my end.

@pedelman
Copy link
Contributor

@hasclass Can you give #123 a shot? If that will satisfy your need, I will go ahead and close this out.

@pedelman
Copy link
Contributor

Going to go ahead and close this for now -- lets move discussions into the open thread safety issue or my PR.

Thanks for opening this PR -- really interesting proposed solution 👍

@pedelman pedelman closed this Jun 22, 2016
@hasclass
Copy link
Author

@pedelman thanks!

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.09%) to 98.81% when pulling 126bc0a on hasclass:master into 837e14e on bigcommerce:master.

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.

5 participants