From 4c9919a23e652b6c82518832f869a38f65c2eae7 Mon Sep 17 00:00:00 2001 From: Thorsten Eckel Date: Wed, 4 Dec 2019 15:29:43 +0100 Subject: [PATCH] ActiveJobLock enhancements (cleanup and Delayed::Job destroy sync) --- app/jobs/active_job_lock_cleanup_job.rb | 7 +++++ ...yed_jobs_ensure_active_job_lock_removal.rb | 20 ++++++++++++++ ...0_active_job_lock_cleanup_job_scheduler.rb | 15 +++++++++++ db/seeds/schedulers.rb | 9 +++++++ ...ive_job_lock_cleanup_job_scheduler_spec.rb | 23 ++++++++++++++++ spec/jobs/active_job_lock_cleanup_job_spec.rb | 27 +++++++++++++++++++ .../jobs/concerns/has_active_job_lock_spec.rb | 17 ++++++++++++ spec/support/active_job.rb | 1 + 8 files changed, 119 insertions(+) create mode 100644 app/jobs/active_job_lock_cleanup_job.rb create mode 100644 config/initializers/delayed_jobs_ensure_active_job_lock_removal.rb create mode 100644 db/migrate/20191129102720_active_job_lock_cleanup_job_scheduler.rb create mode 100644 spec/db/migrate/active_job_lock_cleanup_job_scheduler_spec.rb create mode 100644 spec/jobs/active_job_lock_cleanup_job_spec.rb diff --git a/app/jobs/active_job_lock_cleanup_job.rb b/app/jobs/active_job_lock_cleanup_job.rb new file mode 100644 index 000000000..b9b92a082 --- /dev/null +++ b/app/jobs/active_job_lock_cleanup_job.rb @@ -0,0 +1,7 @@ +class ActiveJobLockCleanupJob < ApplicationJob + include HasActiveJobLock + + def perform(diff = 1.day) + ::ActiveJobLock.where('created_at < ?', Time.zone.now - diff).destroy_all + end +end diff --git a/config/initializers/delayed_jobs_ensure_active_job_lock_removal.rb b/config/initializers/delayed_jobs_ensure_active_job_lock_removal.rb new file mode 100644 index 000000000..627b711d1 --- /dev/null +++ b/config/initializers/delayed_jobs_ensure_active_job_lock_removal.rb @@ -0,0 +1,20 @@ +require 'delayed_job' + +module Delayed + class Job < ::ActiveRecord::Base + + after_destroy :remove_active_job_lock + + def remove_active_job_lock + # only ActiveJob Delayed::Jobs can have a lock + return if !payload_object.is_a?(::ActiveJob::QueueAdapters::DelayedJobAdapter::JobWrapper) + + # deserialize ActiveJob and load it's arguments to generate the lock_key + active_job = ::ActiveJob::Base.deserialize(payload_object.job_data) + active_job.arguments = ::ActiveJob::Arguments.deserialize(active_job.instance_variable_get(:@serialized_arguments)) + + # remove possible lock + active_job.try(:release_active_job_lock!) + end + end +end diff --git a/db/migrate/20191129102720_active_job_lock_cleanup_job_scheduler.rb b/db/migrate/20191129102720_active_job_lock_cleanup_job_scheduler.rb new file mode 100644 index 000000000..ace4fa0c2 --- /dev/null +++ b/db/migrate/20191129102720_active_job_lock_cleanup_job_scheduler.rb @@ -0,0 +1,15 @@ +class ActiveJobLockCleanupJobScheduler < ActiveRecord::Migration[5.2] + def up + return if !Setting.find_by(name: 'system_init_done') + + Scheduler.create_or_update( + name: 'Cleanup ActiveJob locks.', + method: 'ActiveJobLockCleanupJob.perform_now', + period: 1.day, + prio: 2, + active: true, + updated_by_id: 1, + created_by_id: 1, + ) + end +end diff --git a/db/seeds/schedulers.rb b/db/seeds/schedulers.rb index a123b344b..1cd5975d1 100644 --- a/db/seeds/schedulers.rb +++ b/db/seeds/schedulers.rb @@ -127,6 +127,15 @@ Scheduler.create_or_update( updated_by_id: 1, created_by_id: 1, ) +Scheduler.create_or_update( + name: 'Cleanup ActiveJob locks.', + method: 'ActiveJobLockCleanupJob.perform_now', + period: 1.day, + prio: 2, + active: true, + updated_by_id: 1, + created_by_id: 1, +) Scheduler.create_or_update( name: 'Sync calendars with ical feeds.', method: 'Calendar.sync', diff --git a/spec/db/migrate/active_job_lock_cleanup_job_scheduler_spec.rb b/spec/db/migrate/active_job_lock_cleanup_job_scheduler_spec.rb new file mode 100644 index 000000000..905ff7d51 --- /dev/null +++ b/spec/db/migrate/active_job_lock_cleanup_job_scheduler_spec.rb @@ -0,0 +1,23 @@ +require 'rails_helper' + +RSpec.describe ActiveJobLockCleanupJobScheduler, type: :db_migration do + + let(:scheduler_method) { 'ActiveJobLockCleanupJob.perform_now' } + + context 'New system', system_init_done: false do + it 'has no work to do' do + expect { migrate }.not_to change { Scheduler.exists?(method: scheduler_method) }.from(true) + end + end + + context 'System that is already set up' do + + before do + Scheduler.find_by(method: scheduler_method).destroy! + end + + it 'creates Scheduler' do + expect { migrate }.to change { Scheduler.exists?(method: scheduler_method) } + end + end +end diff --git a/spec/jobs/active_job_lock_cleanup_job_spec.rb b/spec/jobs/active_job_lock_cleanup_job_spec.rb new file mode 100644 index 000000000..dcf51a929 --- /dev/null +++ b/spec/jobs/active_job_lock_cleanup_job_spec.rb @@ -0,0 +1,27 @@ +require 'rails_helper' + +RSpec.describe ActiveJobLockCleanupJob, type: :job do + + context 'when ActiveJobLock records older than a day are present' do + + before do + create(:active_job_lock, created_at: 1.day.ago) + travel 1.minute + end + + it 'cleans up those jobs' do + expect { described_class.perform_now }.to change(ActiveJobLock, :count).by(-1) + end + end + + context 'when recent ActiveJobLock records are present' do + + before do + create(:active_job_lock, created_at: 1.minute.ago) + end + + it 'keeps those jobs' do + expect { described_class.perform_now }.not_to change(ActiveJobLock, :count) + end + end +end diff --git a/spec/jobs/concerns/has_active_job_lock_spec.rb b/spec/jobs/concerns/has_active_job_lock_spec.rb index 362d0d23b..df3e310fd 100644 --- a/spec/jobs/concerns/has_active_job_lock_spec.rb +++ b/spec/jobs/concerns/has_active_job_lock_spec.rb @@ -80,6 +80,23 @@ RSpec.describe HasActiveJobLock, type: :job do it 'allows execution of perform_now jobs' do expect { job_class.perform_now }.to change(job_class, :perform_counter).by(1) end + + context 'when Delayed::Job gets destroyed' do + + before do + ::ActiveJob::Base.queue_adapter = :delayed_job + end + + it 'is ensured that ActiveJobLock gets removed' do + job = job_class.perform_later + + expect do + Delayed::Job.find(job.provider_job_id).destroy! + end.to change { + ActiveJobLock.exists?(lock_key: job.lock_key, active_job_id: job.job_id) + }.to(false) + end + end end context 'dynamic lock key' do diff --git a/spec/support/active_job.rb b/spec/support/active_job.rb index 41d2b64f6..f01cfbeff 100644 --- a/spec/support/active_job.rb +++ b/spec/support/active_job.rb @@ -33,6 +33,7 @@ RSpec.configure do |config| example.run + ensure ::ActiveJob::Base.queue_adapter = default_queue_adapter end end