From de593f6e9f9e1363ca53be29a2acdb78572e2716 Mon Sep 17 00:00:00 2001
From: Raphael Sofaer <rafi@sofaer.net>
Date: Thu, 23 Jan 2014 16:47:43 -0800
Subject: [PATCH] Do not retry sending out posts on SSL errors. See #4728
Refactor HydraWrapper so the when-to-retry logic is in the worker
---
app/workers/http_multi.rb | 15 ++++++++++---
lib/hydra_wrapper.rb | 26 ++++++++++++++++------
spec/workers/http_multi_spec.rb | 38 +++++++++++++++++++++++++++++++--
3 files changed, 68 insertions(+), 11 deletions(-)
diff --git a/app/workers/http_multi.rb b/app/workers/http_multi.rb
index 2d633a1fe9..e370cd981b 100644
--- a/app/workers/http_multi.rb
+++ b/app/workers/http_multi.rb
@@ -7,7 +7,10 @@ module Workers
sidekiq_options queue: :http
MAX_RETRIES = 3
-
+ ABANDON_ON_CODES=[:peer_failed_verification, # Certificate does not match URL
+ :ssl_connect_error, # Problem negotiating ssl version or Cert couldn't be verified (often self-signed)
+ :ssl_cacert, # Expired SSL cert
+ ]
def perform(user_id, encoded_object_xml, person_ids, dispatcher_class_as_string, retry_count=0)
user = User.find(user_id)
people = Person.where(:id => person_ids)
@@ -16,11 +19,17 @@ module Workers
hydra = HydraWrapper.new(user, people, encoded_object_xml, dispatcher)
hydra.enqueue_batch
+
+ hydra.keep_for_retry_if do |response|
+ !ABANDON_ON_CODES.include?(response.return_code)
+ end
+
hydra.run
- unless hydra.failed_people.empty?
+
+ unless hydra.people_to_retry.empty?
if retry_count < MAX_RETRIES
- Workers::HttpMulti.perform_in(1.hour, user_id, encoded_object_xml, hydra.failed_people, dispatcher_class_as_string, retry_count + 1)
+ Workers::HttpMulti.perform_in(1.hour, user_id, encoded_object_xml, hydra.people_to_retry, dispatcher_class_as_string, retry_count + 1)
else
Rails.logger.info("event=http_multi_abandon sender_id=#{user_id} failed_recipient_ids='[#{person_ids.join(', ')}] '")
end
diff --git a/lib/hydra_wrapper.rb b/lib/hydra_wrapper.rb
index 562a7ef26e..e22d94d7f9 100644
--- a/lib/hydra_wrapper.rb
+++ b/lib/hydra_wrapper.rb
@@ -17,16 +17,19 @@ class HydraWrapper
}
}
- attr_reader :failed_people, :user, :encoded_object_xml
+ attr_reader :people_to_retry , :user, :encoded_object_xml
attr_accessor :dispatcher_class, :people
delegate :run, to: :hydra
def initialize user, people, encoded_object_xml, dispatcher_class
@user = user
- @failed_people = []
+ @people_to_retry = []
@people = people
@dispatcher_class = dispatcher_class
@encoded_object_xml = encoded_object_xml
+ @keep_for_retry_proc = Proc.new do |response|
+ true
+ end
end
# Inserts jobs for all @people
@@ -38,6 +41,14 @@ class HydraWrapper
end
end
+ # This method can be used to tell the hydra whether or not to
+ # retry a request that it made which failed.
+ # @yieldparam response [Typhoeus::Response] The response object for the failed request.
+ # @yieldreturn [Boolean] Whether the request whose response was passed to the block should be retried.
+ def keep_for_retry_if &block
+ @keep_for_retry_proc = block
+ end
+
private
def hydra
@@ -55,7 +66,7 @@ class HydraWrapper
@people.group_by { |person|
@dispatcher_class.receive_url_for person
}
- end
+ end
# Prepares and inserts job into the hydra queue
# @param url [String]
@@ -83,11 +94,14 @@ class HydraWrapper
event: "http_multi_fail",
sender_id: @user.id,
url: response.effective_url,
- response_code: response.code
+ return_code: response.return_code
}
- message[:response_message] = response.return_message if response.code == 0
Rails.logger.info message.to_a.map { |k,v| "#{k}=#{v}" }.join(' ')
- @failed_people += people_for_receive_url.map(&:id)
+
+ if @keep_for_retry_proc.call(response)
+ @people_to_retry += people_for_receive_url.map(&:id)
+ end
+
end
end
end
diff --git a/spec/workers/http_multi_spec.rb b/spec/workers/http_multi_spec.rb
index f157fa2bd8..b78088e8f1 100644
--- a/spec/workers/http_multi_spec.rb
+++ b/spec/workers/http_multi_spec.rb
@@ -26,13 +26,29 @@ describe Workers::HttpMulti do
code: 200,
body: "",
time: 0.2,
- effective_url: 'http://foobar.com'
+ effective_url: 'http://foobar.com',
+ return_code: :ok
)
@failed_response = Typhoeus::Response.new(
code: 504,
body: "",
time: 0.2,
- effective_url: 'http://foobar.com'
+ effective_url: 'http://foobar.com',
+ return_code: :ok
+ )
+ @ssl_error_response = Typhoeus::Response.new(
+ code: 0,
+ body: "",
+ time: 0.2,
+ effective_url: 'http://foobar.com',
+ return_code: :ssl_connect_error
+ )
+ @unable_to_resolve_response = Typhoeus::Response.new(
+ code: 0,
+ body: "",
+ time: 0.2,
+ effective_url: 'http://foobar.com',
+ return_code: :couldnt_resolve_host
)
end
@@ -56,6 +72,24 @@ describe Workers::HttpMulti do
Workers::HttpMulti.new.perform bob.id, @post_xml, [person.id], "Postzord::Dispatcher::Private"
end
+ it 'retries if it could not resolve the server' do
+ person = @people.first
+
+ Typhoeus.stub(person.receive_url).and_return @unable_to_resolve_response
+
+ Workers::HttpMulti.should_receive(:perform_in).with(1.hour, bob.id, @post_xml, [person.id], anything, 1).once
+ Workers::HttpMulti.new.perform bob.id, @post_xml, [person.id], "Postzord::Dispatcher::Private"
+ end
+
+ it 'does not retry on an SSL error' do
+ person = @people.first
+
+ Typhoeus.stub(person.receive_url).and_return @ssl_error_response
+
+ Workers::HttpMulti.should_not_receive(:perform_in)
+ Workers::HttpMulti.new.perform bob.id, @post_xml, [person.id], "Postzord::Dispatcher::Private"
+ end
+
it 'max retries' do
person = @people.first
--
GitLab