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

Why thread locals? #41

Open
jtmarmon opened this issue Jul 26, 2016 · 12 comments
Open

Why thread locals? #41

jtmarmon opened this issue Jul 26, 2016 · 12 comments

Comments

@jtmarmon
Copy link

Podio uses thread locals to store the client. Is there a reason for doing that instead of standard instance variables? I ask because it causes problems on certain servers.

@jtmarmon
Copy link
Author

jtmarmon@2ada527 Here's a change I made which seems harmless, and also seems to work fine, but would appreciate input on why this decision was made

@theflow
Copy link
Contributor

theflow commented Jul 27, 2016

It's an attempt of making the client thread-safe and avoid leaking the current client to other threads. What are the problems you're seeing?

@jtmarmon
Copy link
Author

@theflow - it's related to #15 . using this in a rails environment where (depending on your server) you have a separate thread or process handling the request means that Podio.client is nil on every request, so we have to make an HTTP request to podio to re-initialize the connection

@cpeters
Copy link
Contributor

cpeters commented Jul 27, 2016

At Podio, part of our application stack is a rails component that leverages the podio-rb library for communication with the API. We have an initializer that establishes the connection and a before_filter in the application_controller.rb which invokes an init method found in the initializer.

@jtmarmon
Copy link
Author

@cpeters we have similar code which does

if Podio.client.nil?
  Podio.setup(...)
  Podio.client.authenticate_with_app(..)
end

but because the client is nil on every request, we're sending an HTTP request to podio for every client request. am I missing something here that would prevent that?

@cpeters
Copy link
Contributor

cpeters commented Jul 28, 2016

Where is this code located and then executed? I'm curious about the conditional statement. Let me dig into the code because this may not actually return nil, in the ruby sense.

def init_podio(username, password) Podio.client = Podio::Client.new(PODIO_CLIENT_OPTIONS) Podio.client.authenticate_with_credentials(username, password) end

That is our client init.

@theflow
Copy link
Contributor

theflow commented Jul 28, 2016

The difference is if you use username/password or initialize the client with a stored token. I'll do some more research but I think we can move the client to use instance variables.

@cpeters
Copy link
Contributor

cpeters commented Jul 28, 2016

I'm missing something here. How does moving to instance variables solve the problem? Each thread will still have to authenticate with an API call.

@jtmarmon
Copy link
Author

@cpeters it's actually not something I understand well and am doing some more research to try and understand, but my fork of podio-rb definitely fixes the issue

@jtmarmon
Copy link
Author

think I figured out why:

class Foo
  class << self
    @@foo = 0
    def bar
      @@foo = 5
    end

    def baz
      @@foo
    end
  end
end

puts Foo.baz
Foo.bar
puts Foo.baz
Thread.new { puts Foo.baz }.join

this prints:

0
5
5

It seems that ruby class variables are both thread safe and globally available

@jtmarmon
Copy link
Author

scratch that, they are not thread safe. but they are available across threads

@cpeters
Copy link
Contributor

cpeters commented Jul 28, 2016

@jtmarmon I think we need to use a mutex.

class Foo
  @mutex = Mutex.new

  def self.bar
    @mutex.synchronize {
      @bar ||= create_no
    }
  end

  def self.create_no
    no = rand(10000)
    sleep 1
    no
  end
end

http://stackoverflow.com/questions/9558192/thread-safety-class-variables-in-ruby

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

No branches or pull requests

3 participants