Issue with "on_thread_error" override

[chiraggshah]

I am trying to override on_thread_error by adding the following in application.rb.

config.solid_queue.on_thread_error = ->(exception) {
  puts "*" * 100
  puts "exception: #{exception}"

  # do something

  Rails.error.report(exception, source: "solid_queue", severity: :error)
}

I have a sample job to trigger the above

class SendMailJob < ApplicationJob
  def perform
    raise StandardError
  end
end

But the proc doesn't get executed when I run SendMailJob.perform_later in the rails console.

[npezza93]

Doesn't seem like on_thread_error is ever called. If I bundle open solid_queue and throw a bunch of puts "here" into the default on_thread_error proc, nothing gets outputted to development.log

[kylekeesling]

Likewise, I have the following config and it does not notify Honeybadger when an error occurs:

config.solid_queue.on_thread_error = ->(exception) {
  Honeybadger.notify(exception, context: {error_message: exception.message, source: "solid_queue"})
}

[rosa]

Hey @chiraggshah, @npezza93, @kylekeesling thanks for reporting this and sorry for the delay. I've been working mostly on unrelated stuff this past week.

on_thread_error wasn't quite intended for errors on the job itself, but rather errors in the thread that's executing the job, but around the job itself. For example, if you had an Active Record's thread pool too small for your number of threads and you got an error when trying to check out a new connection, on_thread_error would be called with that.

For errors in the job itself, you could try to hook into Active Job's itself, similarly to what Sentry's client does here.

[kylekeesling]

@rosa I started to wonder that based on the wording thread instead of job. Thanks for the clarification!

[kylekeesling]

Using the Sentry example Rosa provided, I was able to get the following working in my app by adding it to ApplicationJob:

class ApplicationJob < ActiveJob::Base
  around_perform do |job, block|
    capture_and_record_errors(job, block)
  end

  def capture_and_record_errors(job, block)
    block.call
  # I had to use rescue here instead of a `Rails.error` block because Honeybadger ignores the `Rails.error.report` call
  # in favor of their own error handler, which is fine in most cases, but unfortunately doesn't work here. Report would be
  # great here because it re-raises the error, but instead I have to do that manually
  rescue Exception => e 
    Rails.error.set_context(**error_context(job))
    Rails.error.report(e)
    raise e
  end

  def error_context(job)
    {
      active_job: job.class.name,
      arguments: job.arguments,
      scheduled_at: job.scheduled_at,
      job_id: job.job_id
    }
  end
end

It's important to note that you must include Exception in the error handler as opposed to omitting it or defining StandardError. You also don't have to include the context, but figured it couldn't hurt and was easy to add.

Thanks again @rosa for the wonderful gem and the assistance in figuring this out!

[chiraggshah]

@rosa : Thanks for clarification and for this wonderful gem. Your explanation clears up the confusion I had.

@kylekeesling : Thanks for the above snippet. I used the same and got it working for ApplicationJob by directly calling Honeybadger.notify.

class ApplicationJob < ActiveJob::Base
  around_perform do |job, block|
    capture_and_record_errors(job, block)
  end

  def capture_and_record_errors(job, block)
    block.call
  rescue => exception
    context = {
      error_class: job.class.name,
      args: job.arguments,
      scheduled_at: job.scheduled_at,
      job_id: job.job_id
    }
    Honeybadger.notify(exception, context:)
    raise exception
  end
end

I also added similar code to the around block for ActionMailer::MailDeliveryJob to make it work for Mailers.

# application_mailer.rb

ActionMailer::MailDeliveryJob.around_perform do |job, block|
  block.call
rescue => exception
  context = {
    error_class: job.class.name,
    args: job.arguments,
    scheduled_at: job.scheduled_at,
    job_id: job.job_id
  }
  Honeybadger.notify(exception, context:)
  raise exception
end

While searching for ways on why just adding it to application_job.rb wasn't working for mailers, I came across this section in the readme of good_job gem which helped clear up the confusion and provide the above solution for mailers.

@rosa : Do you think it would be a good thing to add both of the above in the readme? If yes, then I would be happy to raise a PR.

[rosa]

@rosa : Do you think it would be a good thing to add both of the above in the readme? If yes, then I would be happy to raise a PR.

Oops, sorry for the delay! I missed this mention. I think that'd be super helpful, please do raise a PR if you have time 🙏 If not, I'll address it soon.

[chiraggshah]

@rosa : I have raised a PR https://github.com/basecamp/solid_queue/pull/139

[rosa]

So many months later, I updated and merged that PR 😅 I kept debating whether to change the behaviour or document the existing behaviour, and in the end, with the v1.0.0 pressure, went for documenting for now 😅