Is Memoist Thread Safe?

[benlieb]

I'm trying to make my app thread-safe to use the Puma server.

I'm using this in my payments code and don't want to get this wrong!

app/models/concerns/user_stripe_payments_module.rb:8: memoize :payment_profile # reload and rememoizes with #payment_profile(true)
app/models/concerns/user_stripe_payments_module.rb:15: def rememoize_payment_profile

[matthewrudy]

@benlieb that depends what you mean by thread-safe.
It doesn't synchronize calls to the cache.

But it depends what objects your memoizing,
and how they're used across threads.

At the end of the day memoist is mostly just the same as taking

def my_method
  do_something
end

And changing it to

def my_method
  @__my_method ||= do_something
end

If you really wanted to make this "safe"
you'd have to do

def initialize(*args)
  @__my_method_mutex = Mutex.new
end

def my_method
  @__my_method_mutex.synchronize do
    @__my_method ||= do_something
  end
end

but that seems crazy

Maybe you can write a minimal example of what you're trying to do,
and I can comment on whether its "safe"

[benlieb]

Basically I want to run my app with Puma on Heroku, which requires "thread-safety". Looking through several articles on the subject, both memoizing and using ||= can be issues when aiming for thread safety. I'm not an expert, but the two articles that I read in-depth were:

https://bearmetal.eu/theden/how-do-i-know-whether-my-rails-app-is-thread-safe-or-not/
http://thoughts.codegram.com/understanding-class-instance-variables-in-ruby/

I'm not doing anything fancy, but I just want to ensure that all my gems (not picking on you guys :) are thread-safe. In my case the worse case would be one user somehow making a payment with another user's payment profile, as shown above, but here is the code:

class User < ActiveRecord::Base
  extend Memoist
  include UserStripePaymentsModule
  ...
end

# consolidates payment-realted methods, in this case for Stripe
# try to consider method names that are generic to continue to be used if
# stripe is swapped for a different payment processor in future
module UserStripePaymentsModule 
  extend ActiveSupport::Concern

  included do
    memoize :payment_profile # reload and rememoizes with #payment_profile(true)
  end

  def payment_profile
    Stripe::Client.instance.customer_get self
  end

  def rememoize_payment_profile
    payment_profile(true)
  end

  def has_cc_on_file?
    !!cc_info
  end

  def cc_info
    return nil unless has_payment_profile? 
    payment_profile.sources.data.first
  end

  def cc_last4
    cc_info && cc_info.last4
  end

  def has_payment_profile?
    stripe_customer_id.present?
  end
end

[matthewrudy]

Ok, so to simplify your example, cut memoist out of it.

class User < ActiveRecord::Base
  def payment_profile
    @payment_profile ||= Stripe::Client.instance.customer_get self
  end
end

So this is memoized for an instance of user

Two threads running on Puma may do something like this:

Thread#1 user = User.find 123; user.payment_profile
Thread#2 user = User.find 123; user.payment_profile

But these two user objects are different.
So actually this doesn't matter.

What you do need to do is ensure that your classes are loaded at boot
or you could have race conditions where loading user.rb happened twice, and memoized it twice.

But rails loading handles this for you.

[benlieb]

This is a rails app, so the loading issue should be taken care of, or am I wrong?

[matthewrudy]

@benlieb yeah.

[matthewrudy]

An example of how you could mess this up.

user = User.find 123
5.times do 
  Thread.new do
    5.times do
      user.payment_profile
      user.payment_profile(true)
    end
  end
end

but that's only because you're using a shared object across threads,
and not synchronizing state.

[Muriel-Salvan]

+1 on having it thread-safe, as in the example provided by @matthewrudy .
It could be optional (like having memoize and memoize_threadsafe methods) as I guess having every call protected by a mutex could be harmful performance wise. A developer should know which of its methods should be thread-safe, and not all of them should be. Hence having the possibility to switch this option per memoized method might be good.