Commit e3b94a6d authored by Sean McGivern's avatar Sean McGivern

Avoid using Timeout when deleting Sidekiq jobs

Deleting a Sidekiq job calls out to Redis, but Timeout.timeout is not
safe for network calls:

https://www.mikeperham.com/2015/05/08/timeout-rubys-most-dangerous-api/
parent 1985328c
...@@ -14,7 +14,8 @@ module Gitlab ...@@ -14,7 +14,8 @@ module Gitlab
end end
def drop_jobs!(search_metadata, timeout:) def drop_jobs!(search_metadata, timeout:)
completed = false start_time = Gitlab::Metrics::System.monotonic_time
completed = true
deleted_jobs = 0 deleted_jobs = 0
job_search_metadata = job_search_metadata =
...@@ -27,18 +28,16 @@ module Gitlab ...@@ -27,18 +28,16 @@ module Gitlab
raise NoMetadataError if job_search_metadata.empty? raise NoMetadataError if job_search_metadata.empty?
raise InvalidQueueError unless queue raise InvalidQueueError unless queue
begin queue.each do |job|
Timeout.timeout(timeout) do if timeout_exceeded?(start_time, timeout)
queue.each do |job| completed = false
next unless job_matches?(job, job_search_metadata) break
end
job.delete next unless job_matches?(job, job_search_metadata)
deleted_jobs += 1
end
completed = true job.delete
end deleted_jobs += 1
rescue Timeout::Error
end end
{ {
...@@ -48,6 +47,8 @@ module Gitlab ...@@ -48,6 +47,8 @@ module Gitlab
} }
end end
private
def queue def queue
strong_memoize(:queue) do strong_memoize(:queue) do
# Sidekiq::Queue.new always returns a queue, even if it doesn't # Sidekiq::Queue.new always returns a queue, even if it doesn't
...@@ -59,5 +60,9 @@ module Gitlab ...@@ -59,5 +60,9 @@ module Gitlab
def job_matches?(job, job_search_metadata) def job_matches?(job, job_search_metadata)
job_search_metadata.all? { |key, value| job[key] == value } job_search_metadata.all? { |key, value| job[key] == value }
end end
def timeout_exceeded?(start_time, timeout)
(Gitlab::Metrics::System.monotonic_time - start_time) > timeout
end
end end
end end
...@@ -31,14 +31,7 @@ describe Gitlab::SidekiqQueue do ...@@ -31,14 +31,7 @@ describe Gitlab::SidekiqQueue do
context 'when the queue is not processed in time' do context 'when the queue is not processed in time' do
before do before do
calls = 0 allow(Gitlab::Metrics::System).to receive(:monotonic_time).and_return(1, 2, 12)
allow(sidekiq_queue).to receive(:job_matches?).and_wrap_original do |m, *args|
raise Timeout::Error if calls > 0
calls += 1
m.call(*args)
end
end end
it 'returns a non-completion flag, the number of jobs deleted, and the remaining queue size' do it 'returns a non-completion flag, the number of jobs deleted, and the remaining queue size' do
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment