diff options
author | Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com> | 2015-08-25 12:23:43 +0300 |
---|---|---|
committer | Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com> | 2015-08-25 12:23:43 +0300 |
commit | c1b490d6ef995c9d0baf0133a96f13e4757214b6 (patch) | |
tree | 0167619da83fa70212f1dd6d264e852e667968c9 | |
parent | c06cf2bde6fcb08f476d6cf9180c545b757ce829 (diff) | |
parent | 001c8cd0ee6e70cc96727cc37eedf263b916d24b (diff) |
Merge branch 'handle-smtp-input-errors' into 'master'
Gracefully handle SMTP user input errors (e.g. incorrect email addresses) to prevent Sidekiq retries
### What does this MR do?
This MR gracefully handles SMTP input errors (e.g. incorrect or invalid e-mail addresses) to prevent these types of exceptions from causing Sidekiq to retry the task. If these specific exceptions occur, they will be logged, and the e-mail will be dropped from the queue.
### Why was this MR needed?
If you include an author that has a misspelled e-mail address, Sidekiq will keep sending e-mail to all the recipients even if they have already received the e-mail. The only way to recover is to clear the Sidekiq queue.
Note that other exceptions can still be thrown (e.g. `IOError`, `Net::SMTPAuthenticationError`, `Net::SMTPServerBusy`, `Net::SMTPUnknownError`, and `TimeoutError`). If the worker encounters these, Sidekiq should retry the task.
### What are the relevant issue numbers?
Closes https://github.com/gitlabhq/gitlabhq/issues/9560
See merge request !1163
-rw-r--r-- | CHANGELOG | 1 | ||||
-rw-r--r-- | app/workers/emails_on_push_worker.rb | 33 | ||||
-rw-r--r-- | spec/workers/emails_on_push_worker_spec.rb | 34 |
3 files changed, 54 insertions, 14 deletions
diff --git a/CHANGELOG b/CHANGELOG index 9fddcf82783..35b5803d341 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -13,6 +13,7 @@ v 8.0.0 (unreleased) - Search for comments should be case insensetive - Create cross-reference for closing references on commits pushed to non-default branches (Maƫl Valais) - Ability to search milestones + - Gracefully handle SMTP user input errors (e.g. incorrect email addresses) to prevent Sidekiq retries (Stan Hu) v 7.14.1 (unreleased) - Only include base URL in OmniAuth full_host parameter (Stan Hu) diff --git a/app/workers/emails_on_push_worker.rb b/app/workers/emails_on_push_worker.rb index 1d21addece6..916a99bb273 100644 --- a/app/workers/emails_on_push_worker.rb +++ b/app/workers/emails_on_push_worker.rb @@ -4,7 +4,7 @@ class EmailsOnPushWorker def perform(project_id, recipients, push_data, options = {}) options.symbolize_keys! options.reverse_merge!( - send_from_committer_email: false, + send_from_committer_email: false, disable_diffs: false ) send_from_committer_email = options[:send_from_committer_email] @@ -16,9 +16,9 @@ class EmailsOnPushWorker ref = push_data["ref"] author_id = push_data["user_id"] - action = + action = if Gitlab::Git.blank_ref?(before_sha) - :create + :create elsif Gitlab::Git.blank_ref?(after_sha) :delete else @@ -42,17 +42,22 @@ class EmailsOnPushWorker end recipients.split(" ").each do |recipient| - Notify.repository_push_email( - project_id, - recipient, - author_id: author_id, - ref: ref, - action: action, - compare: compare, - reverse_compare: reverse_compare, - send_from_committer_email: send_from_committer_email, - disable_diffs: disable_diffs - ).deliver + begin + Notify.repository_push_email( + project_id, + recipient, + author_id: author_id, + ref: ref, + action: action, + compare: compare, + reverse_compare: reverse_compare, + send_from_committer_email: send_from_committer_email, + disable_diffs: disable_diffs + ).deliver + # These are input errors and won't be corrected even if Sidekiq retries + rescue Net::SMTPFatalError, Net::SMTPSyntaxError => e + logger.info("Failed to send e-mail for project '#{project.name_with_namespace}' to #{recipient}: #{e}") + end end ensure compare = nil diff --git a/spec/workers/emails_on_push_worker_spec.rb b/spec/workers/emails_on_push_worker_spec.rb new file mode 100644 index 00000000000..3600c771075 --- /dev/null +++ b/spec/workers/emails_on_push_worker_spec.rb @@ -0,0 +1,34 @@ +require 'spec_helper' + +describe EmailsOnPushWorker do + include RepoHelpers + + let(:project) { create(:project) } + let(:user) { create(:user) } + let(:data) { Gitlab::PushDataBuilder.build_sample(project, user) } + + subject { EmailsOnPushWorker.new } + + before do + allow(Project).to receive(:find).and_return(project) + end + + describe "#perform" do + it "sends mail" do + subject.perform(project.id, user.email, data.stringify_keys) + + email = ActionMailer::Base.deliveries.last + expect(email.subject).to include('Change some files') + expect(email.to).to eq([user.email]) + end + + it "gracefully handles an input SMTP error" do + ActionMailer::Base.deliveries.clear + allow(Notify).to receive(:repository_push_email).and_raise(Net::SMTPFatalError) + + subject.perform(project.id, user.email, data.stringify_keys) + + expect(ActionMailer::Base.deliveries.count).to eq(0) + end + end +end |