PSA: do not use instance variables in event handlers

[pch]

Hey all,

(First off, this issue was 100% our fault and it's not a bug in RES. But I'm posting this here in case anyone runs into the same problem)

We were migrating some of our existing code to RES and in one of the handlers ended up with something like (simplified):

class HandleNewContactAssigned < ::Infra::EventHandler
  def call(event)
    @contact = Contact.find(event.data.fetch(:contact_id))
    @agency = Agency.find(event.data.fetch(:agency_id))
    
    send_notifications
  end

  private

    attr_reader :contact, :agency

    def send_notifications
       agency.users.each do |u|
         send_email_to(u, contact)
       end
    end
end

The event handler is instantiated once, when app initializes:

Rails.configuration.event_store.tap do |store|
  store.subscribe(HandleNewContactAssigned.new, to: [SomeEvent])
end

Normally, this wouldn't be a problem, but we have a bunch of Sidekiq jobs that fire up exactly at 8am each day and run concurrently. Because there's only 1 instance of this handler, we started sending emails randomly to wrong people...

This is one of those "duh, of course" bugs (in hindsight), but just wanted to put it out there.

We added custom Rubocop rules to avoid repeating this bug. Feel free to steal:

# rubocop.yml
require:
  - rubocop-performance
  - ./lib/custom_rubocops/cop_base.rb
  - ./lib/custom_rubocops/no_instance_variable_in_event_handler.rb
  - ./lib/custom_rubocops/no_attr_reader_in_event_handler.rb

module CustomRubocops
  class CopBase < RuboCop::Cop::Base
    private

      def event_handler_class?(node)
        return false unless node.parent_class

        node.parent_class.const_name == 'Infra::EventHandler'
      end
  end
end

module CustomRubocops
  class NoAttrReaderInEventHandler < CopBase
    MSG = 'Do not use attr_reader in Infra::EventHandler classes.'.freeze

    def on_class(node)
      return unless event_handler_class?(node)

      check_for_attr_reader(node)
    end

      private

        def check_for_attr_reader(node)
          node.each_descendant(:send) do |child_node|
            if child_node.method_name == :attr_reader
              add_offense(child_node, message: MSG)
            end
          end
        end
  end
end

module CustomRubocops
  class NoInstanceVariableInEventHandler < CopBase
    MSG = 'Do not use instance variables or attr_reader in Infra::EventHandler classes.'.freeze

    def on_class(node)
      return unless event_handler_class?(node)

      check_for_instance_variables(node)
    end

    private

      def check_for_instance_variables(node)
        node.each_descendant(:ivasgn, :ivar) do |child_node|
          add_offense(child_node, message: MSG)
        end
      end
  end
end

[mostlyobvious]

Good story about the race and concurrently modifying instance variables over the same instance!

[mostlyobvious]

Btw. I'm seeing familiar ::Infra::EventHandler class name, of unknown implementation and subscribe(HandleNewContactAssigned.new, ...) which tells me this is a synchronous handler.

Just making sure this was intended — not an asynchronous one.

[pch]

Yes, this is a synchronous handler and it's intentional. Whether it's a correct approach, that's another issue 😉