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

Documentation: Warning against memoization #122

Open
m5rk opened this issue Jun 28, 2019 · 6 comments
Open

Documentation: Warning against memoization #122

m5rk opened this issue Jun 28, 2019 · 6 comments

Comments

@m5rk
Copy link

m5rk commented Jun 28, 2019

There's only one subscriber instantiated from this configure block:

StripeEvent.configure do |events|
  events.subscribe 'invoice.', StripeInvoiceEvent.new
end

And that means that you can't have methods in StripeInvoiceEvent that memoize:

def account
  @account || = Account.find_by(stripe_customer_id: event.data.object.id)
end
@rmm5t
Copy link
Member

rmm5t commented Jun 30, 2019

A documentation PR is welcome, though it should probably be more specific about the fact that event subscriptions are singletons that just respond to call.

With that said, and/or alternatively, and/or optimally, I suppose there's also room to adjust the StripeEvent::NotificationAdapter#call to instantiate a new instance if the passed subscriber is a Class as opposed to an instance of something that responds to #call itself. I'd be willing to review a PR (with specs) that does that.

@yannis
Copy link

yannis commented Aug 16, 2019

First, I'd like to thank you for the great gem which helped us a lot while integrating Stripe in our app.

Still, we were victim of the memoization issue, and I'd like to insist on the fact that the use of a Singleton, instantiated in an initializer can lead to a lot of issues.
Indeed, any setting of an instance variable in the recorder is a mistake since it can change anytime, even in the middle of a method call.
For example, we had something like this (code simplified for clarity):

class StripeInvoiceRecorder
  attr_reader :action, event

  def initialize(action:)
    @action = action
  end

  def call(event)
    @event = event
     send(action)
  end

  private def payment_succeeded
    object = event["data"]["object"]
    SubscriptionJob1.perform_later(subscription_stripe_id: object["subscription"])
    SubscriptionJob2.perform_later(stripe_invoice_id: object["id"])
  end
end

Here, instantiating @event = event in #call is a mistake as any other call to #call during the execution of #payment_succeeded will change the value of @event changing object mid-flight.

So to prevent this issue, we changed this code to:

class StripeInvoiceRecorder
  attr_reader :action

  def initialize(action:)
    @action = action
  end

  def call(event)
     send(action, event)
  end

  private def payment_succeeded(event)
    object = event["data"]["object"]
    SubscriptionJob1.perform_later(subscription_stripe_id: object["subscription"])
    SubscriptionJob2.perform_later(stripe_invoice_id: object["id"])
  end
end

I hope it will help other having strange issues with their recorders.
And indeed, switching from a Singleton to instance variables would prevent this kind of issues.

@vakrolme
Copy link

+1, and I would expand this to code reloading also, since I've just spent some hours figuring out why code reloading doesn't work with a service object that I pass into the above mentioned initializer (thought it to be a Zeitwerk issue or something).

In hindsight it all makes sense. But it isn't obvious if you just mindlessly follow the setup guide.

@rromanchuk
Copy link

BTW Using StripeEvent.configure do |events| in a config/initializers/stripe.rb (as shown in the README) and assigning classes that live in /app is going to cause problems in rails6+

@kirylrb
Copy link

kirylrb commented Feb 1, 2021

@rromanchuk What kind of problems you mean?

@christianrolle
Copy link

@kirylpl related to the subscribed class, like:

StripeEvent.configure do |events|
  events.subscribe 'charge.failed', Stripe::ChargeFailedInteractor
end

and

module Stripe
  class FailedChargeInteractor
    include Interactor
    
    def call(event)
      Rails.logger.error("Stripe charge failed: #{params}")
    end
  end
end

and then running the server with rails s:

uninitialized constant Stripe::ChargeFailedInteractor (NameError)

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

7 participants