An error raised in an ActiveDelivery line behaves differently than one raised in an ActiveMailer line about abstract_notifier

[thornomad]

@dmitry — I ended up stopping using AbstractNotifier and just rolled my own delivery line based on the recommendations at the ActiveDelivery README. I didn't see a clear pathway to easily converting this gem to do what I wanted without more work than just writing our own. Probably need to break this into two different classes/modules that a user can inherit from like AbstractNotifier::Base and AbstractNotifer::LazyBase or something. Heh.

[palkan]

Probably need to break this into two different classes/modules that a user can inherit from like AbstractNotifier::Base and AbstractNotifer::LazyBase or something

I like it. Though, maybe, we can do a little bit different thing: build a new lazy line and use it for AbstractNotifier.
Smth like this:

# stays the same
def notify_now(handler, mid, *args)
  handler.public_send(mid, *args).notify_now
end

# use a separate job
def notify_later(handler, mid, *args)
   handler.class.async_adapter.enqueue_raw(handler.class, params, notification_name, args)
end

module AbstractNotifier
  module AsyncAdapters
    class ActiveJob
      class RawDeliveryJob < ::ActiveJob::Base
        def perform(notifier_class_name, params, action, payload)
           notifier_class = notifier_class_name.constantize
           notifier = notifier_class.new(action, **params)
           notifier.public_send(action, *payload).notify_now
        end
      end
    end
  end
end

[palkan]

Hey @thornomad!

Your setup looks fine.

I would have expected them all to behave like ActiveMailer — they aren't actually executed until after the job is queued.

There is a difference in the way ActionMailer and AbstractNotifier. Let's see how they work:

• ActionMailer creates an intermediate MessageDelivery object whenever you call MyMailer.some_method; then you call #deviler_later, which enqueues the job with this message delivery object; the action itself hasn't been processed yet, we pass class, params and arguments to the job and only invoke the action inside it.
• AbstractNotifier also uses an intermediate object, Notification, but unlike ActionMailer, this object performs the action immediately to calculate the payload for the notifier adapter. Thus, we only enqueue the notifier class (to get the adapter) and the final payload.

tl;dr ActionMailer lazily calculate the payload, while AbstractNotifier does not.

That was intentional at the time I've extracted this library from the commercial project: notifier payloads were pretty small and cheap to calculate, so it didn't make any sense to calculate them lazily (and thus, make the implementation a bit more complex).

Can we refactor the internals to process notifications lazily? Yeah, I think, we can. Probably, we should make it configurable per notifier class.

[thornomad]

Hi @palkan — thanks for the detailed response. Your description fits with the behavior I was seeing about "immediately calculating the payload". For our setup, we have an ActiveJob is calling FooBarDelivery.notify at the end of it. If there is an error during the immediate part of the execution in an AbstractDelivery line it is causing the whole job to re-run (and all the lines re-run up to the failure) which is resulting in a lot of spam notifications. ActiveMailer, as you point out, doesn't have this behavior.

For the project I am working on, we are going to need the lazy execution (one way or another). I haven't dug too much into the codebase yet, but if you are willing to have a flag that also you to designate some notifiers as "lazy" versus others I'm happy to look into sorting it out. And if you have an idea about how it could be done effectively let me know!

[dmitry]

@thornomad @palkan would be great to continue to work on this issue. Do you have an existing solutions, which can be embedded into this gem?