FIX: Handle SMTPServerBusy for group smtp email (#13632)

Use the `sidekiq_retry_in` code from Jobs::UserEmail in group SMTP. Also we don't need to keep `seconds_to_delay` -- sidekiq uses the default delay calculation if you return 0 or nil from the block. See 3330df0ee3/lib/sidekiq/job_retry.rb (L216-L234) for sidekiq default retry delay logic.

I experimented with extracting this into a concern or a module, but `sidekiq_retry_in` is quite magic and it would not allow me to abstract away into a module that calls some method specificall in the child job class.

I would love to write tests for this, but it does not seem possible (not sure if its because of our test
setup) to write tests that test sidekiq's retry capability, and I am not sure if we should be anyway. Initial addition
to UserEmail did not test this functionality 
d224966a0e
This commit is contained in:
Martin Brennan 2021-07-06 13:37:52 +10:00 committed by GitHub
parent 38332cae21
commit b3d3ad250b
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
3 changed files with 31 additions and 19 deletions

View File

@ -8,6 +8,19 @@ module Jobs
sidekiq_options queue: 'critical'
sidekiq_retry_in do |count, exception|
# retry in an hour when SMTP server is busy
# or use default sidekiq retry formula. returning
# nil/0 will trigger the default sidekiq
# retry formula
#
# See https://github.com/mperham/sidekiq/blob/3330df0ee37cfd3e0cd3ef01e3e66b584b99d488/lib/sidekiq/job_retry.rb#L216-L234
case exception.wrapped
when Net::SMTPServerBusy
return 1.hour + (rand(30) * (count + 1))
end
end
def execute(args)
return if quit_email_early?

View File

@ -12,11 +12,13 @@ module Jobs
sidekiq_options retry: RETRY_TIMES.size
sidekiq_retry_in do |count, exception|
# returning nil/0 will trigger the default sidekiq
# retry formula
#
# See https://github.com/mperham/sidekiq/blob/3330df0ee37cfd3e0cd3ef01e3e66b584b99d488/lib/sidekiq/job_retry.rb#L216-L234
case exception.wrapped
when SocketError
RETRY_TIMES[count]
else
::Jobs::UserEmail.seconds_to_delay(count)
return RETRY_TIMES[count]
end
end

View File

@ -8,6 +8,19 @@ module Jobs
sidekiq_options queue: 'low'
sidekiq_retry_in do |count, exception|
# retry in an hour when SMTP server is busy
# or use default sidekiq retry formula. returning
# nil/0 will trigger the default sidekiq
# retry formula
#
# See https://github.com/mperham/sidekiq/blob/3330df0ee37cfd3e0cd3ef01e3e66b584b99d488/lib/sidekiq/job_retry.rb#L216-L234
case exception.wrapped
when Net::SMTPServerBusy
return 1.hour + (rand(30) * (count + 1))
end
end
# Can be overridden by subclass, for example critical email
# should always consider being sent
def quit_email_early?
@ -206,22 +219,6 @@ module Jobs
[message, nil]
end
sidekiq_retry_in do |count, exception|
# retry in an hour when SMTP server is busy
# or use default sidekiq retry formula
case exception.wrapped
when Net::SMTPServerBusy
1.hour + (rand(30) * (count + 1))
else
::Jobs::UserEmail.seconds_to_delay(count)
end
end
# extracted from sidekiq
def self.seconds_to_delay(count)
(count**4) + 15 + (rand(30) * (count + 1))
end
private
def skip_message(reason)