1
\$\begingroup\$

I have a task that checks for campaigns that are supposed to be finished by checking the end_at date. If finished, then set the status of the campaign to finished, and send the owner of the campaign an email.

The way I see it, is approach 1 and 2 may have the worker queued more than once in the worker pool.

Approach 3, always makes sure that there is only one instance running at a time. Approach 3 also means making sure the worker is run once on every deploy.

I have 3 scenarios to consider implementing this:

APPROACH #1 - Run the cron job. Cron job determines which campaigns the worker should process

# CRON via whenever gem that runs every 3 minutes
campaigns = Campaign.where("properties -> 'end_at' <= ? AND status = ?", Time.now.utc, 'running')

campaigns.each do |campaign|
  FinishCampaignWorker.perform_async(campaign.id)
end

# FinishCampaignWorker class
class FinishCampaignWorker
  include Sidekiq::Worker

  sidekiq_options({
    retry: false,
    backtrace: true
  })

  def perform(campaign_id)
    begin
      campaign = Campaign.unscoped.find(campaign_id)

      # Do not update campaign if deleted or deactivated
      if campaign.present? && !%w(deleted deactivated).include?(campaign.status)
        campaign.status = 'finished'

        UserMailer.post_campaign_notification(campaign.id, campaign.user.id).deliver

        campaign.save
      end
    rescue
      Airbrake.notify(
        error_class:      'Finish campaign worker',
        error_message:    "#{$!}",
        environment_name: Rails.env
      )
    end
  end
end

APPROACH #2 - Cron triggers the worker to run every 3 minutes. Let the worker handle most of the logic

# CRON via whenever gem that runs every 3 minutes
FinishCampaignWorker.perform_async

# FinishCampaignWorker class
class FinishCampaignWorker
  include Sidekiq::Worker

  sidekiq_options({
    retry: false,
    backtrace: true
  })

  def perform
    begin
      campaigns = Campaign.where("properties -> 'end_at' <= ? AND status = ?", Time.now.utc, 'running')

      campaigns.each do |campaign|
        campaign = Campaign.unscoped.find(campaign_id)

        # Do not update campaign if deleted or deactivated
        if !%w(deleted deactivated).include?(campaign.status)
          campaign.status = 'finished'

          UserMailer.post_campaign_notification(campaign.id, campaign.user.id).deliver

          campaign.save
        end
      end
    rescue
      Airbrake.notify(
        error_class:      'Finish campaign worker',
        error_message:    "#{$!}",
        environment_name: Rails.env
      )
    end
  end
end

APPROACH #3 - Run the worker from console/chef/other on first deployment, let the worker handle most of the logic, and then reschedule the worker 3 minutes after completion

# FinishCampaignWorker class
class FinishCampaignWorker
  include Sidekiq::Worker

  sidekiq_options({
    retry: false,
    backtrace: true
  })

  def perform
    begin
      campaigns = Campaign.where("properties -> 'end_at' <= ? AND status = ?", Time.now.utc, 'running')

      campaigns.each do |campaign|
        campaign = Campaign.unscoped.find(campaign_id)

        # Do not update campaign if deleted or deactivated
        if !%w(deleted deactivated).include?(campaign.status)
          campaign.status = 'finished'

          UserMailer.post_campaign_notification(campaign.id, campaign.user.id).deliver

          campaign.save
        end
      end
    rescue
      Airbrake.notify(
        error_class:      'Finish campaign worker',
        error_message:    "#{$!}",
        environment_name: Rails.env
      )
    end

    FinishCampaignWorker.perform_at(3.minutes.from_now)
  end
end
\$\endgroup\$
1
\$\begingroup\$

Your problem is an integration and management problem, rather than a programming problem (though there are some issues there too).

Approach 3

Approach-3 is a problem, and I would not use it for your task. Repetitive tasks require a well designed and reliable infrastructure to handle them. The quirks of time are such that things like daylight-saving, system clock resets, and other issues will play havoc with things. For example, What will your task do when you 'spring forward' in daylight savings... will it fail because the next scheduled time never happens? In fall, will it go idle for over an hour when the same hour is repeated?

Cron (and on many Linux distributions, Anacron) has very well documented practices and behaviours for these types of issues, and the predictable, and reasonable use case makes a lot of sense. Use Cron.

Additionally, if there is an issue sending a mail, or anything requiring rescue in Approach 3, you will fail to reschedule, and your program will break.

Approach 2

This is actually quite similar to Approach 1, but it just shifts more logic in to the process method, and it mixes up the responsibilities in that method. SOLID principles suggest having just a single responsibility in each method, so I don't like this approach. Additionally, you can only process one campaign at a time using this approach.

Too much happening in one place, and possible opportunities for parallelism are removed.

Approach 1

I prefer this approach for the relative simplicity of the single-campaign method, and the way the campaigns are processed.

Improvements

Having said that, there are problems still. The biggest is that your program is not defensive enough. Even though you expect only one instance of the program to be running at any one time, you should still program defensively and assume there could be more.

Cron (anacron) will ensure that it does not start a second instance of the program if the previous instance has not yet completed, so cron will not cause a problem, but you may have some mix up happening in your testing, or you may run it yourself when cron has it running already, leading to problems from there.

I am not very familiar with RoR, but the following looks like it could lead to conflicts and race conditions with other running processes:

  # Do not update campaign if deleted or deactivated
  if campaign.present? && !%w(deleted deactivated).include?(campaign.status)
    campaign.status = 'finished'

    UserMailer.post_campaign_notification(campaign.id, campaign.user.id).deliver

    campaign.save
  end

Posting the campaign e-mail could be a slow task and (even if it was fast) there is the possibility that another instance of the program may be processing the same campaign at the same time. If you are using a database for your campaigns you should consider a different approach. I recommend:

  • a transactional approach which checks to see whether the campaign is state='finished', updates it to finished, and returns the previous state, and then commits the transaction.
  • this guarantees the state will be marked as finished in a single 'atomic' operation
  • it saves the Campaign state before sending the mail.
  • there is only a short period of time while the campaign is 'locked'
  • if the previous state was not 'finished' then send the mail.

At worst case, now, the only risk is that the mail fails to send. That should be logged in a reliable way that allows you to (manually?) identify, resolve, and resend the mails.

Note that a failed mail will fail whether you mark the Campaign complete or not, and repeatedly resending the mail will not make it suddenly work. There is no reason to save the campaign after the mail-send.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Doesn't Ruby Time handle DST? rxr.whitequark.org/mri/source/time.c#3655 In this case doing something like 1.hour.from_now takes DST into account. Correct me if I am wrong \$\endgroup\$ – Christian Fazzini Dec 10 '14 at 16:02
  • \$\begingroup\$ "if there is an issue sending a mail, or anything requiring rescue in Approach 3, you will fail to reschedule, and your program will break." If the rescue block is triggered (due to any problem), then the consequences are the same for all approaches (i.e. the jobs will never get processed correctly, even if the Cron job triggers the workers. for #1 and #2). I don't think approach #3 will have any special treatment in this regard. \$\endgroup\$ – Christian Fazzini Dec 10 '14 at 16:06
  • \$\begingroup\$ @ChristianFazzini - in approach 3, if the mail fails, then there will be no reschedule. In 1 and 2, the reschedule is handled by cron. \$\endgroup\$ – rolfl Dec 10 '14 at 16:07
  • \$\begingroup\$ Ok that makes sense. I missed that part. One other approach I forgot to mention that I could apply for #1, is to create a mutex block to prevent multiple instances of the worker from running. What are your thoughts on this? \$\endgroup\$ – Christian Fazzini Dec 10 '14 at 16:11
  • \$\begingroup\$ A mutex block would have to ock on something outside the code. It could work, but, in my experience, the better option is to allow parallel execution, and to handle it gracefully. This allows the best of both worlds. This allows, for example, having multiple hosts running against a single central database, etc. \$\endgroup\$ – rolfl Dec 10 '14 at 16:14

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.