From 66125d123cff8742e729503dfdd1cc2dac7bb34a Mon Sep 17 00:00:00 2001 From: Nathan Griffith Date: Fri, 20 Jan 2023 17:02:09 -0500 Subject: [PATCH] Reduce handler size in some ActiveJob cases, and fix deserialization inconsistency (#24) The issue here is that some ActiveJob-enqueued jobs would include the `@job` ivar in their serialized handler, which is redundant with `@job_data` and only existed for memoization purposes. We can use Ruby's `encode_with` API to ensure that we only encode `@job_data` into the handler. Including `@job` also had the side-effect of causing `DeserializationErrors` and permanently failing jobs in cases where there would otherwise be a retryable `NameError`. Whether a missing constant _should_ result in a `DeserializationError` is kind of a separate question, but since `ActiveJob`'s stance seems to be to encode the class name as a string and only `.constantize` it inside of the perform, I think that it's not surprising behavior to let the `NameError` (and subsequent retries) occur. `DeserializationError` is really supposed to be only for cases where we fundamentally can't even attempt the job and allow the normal pattern of exception-handling to occur. --- .rubocop_todo.yml | 77 +++++++++++++++++++++---- Appraisals | 6 ++ CHANGELOG.md | 8 +++ delayed.gemspec | 2 +- gemfiles/rails_5_2.gemfile | 2 + gemfiles/rails_6_0.gemfile | 2 + gemfiles/rails_6_1.gemfile | 2 + lib/delayed/active_job_adapter.rb | 4 ++ spec/delayed/active_job_adapter_spec.rb | 34 +++++++++++ 9 files changed, 125 insertions(+), 12 deletions(-) diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index c2dcf3ef..73641032 100644 --- a/.rubocop_todo.yml +++ b/.rubocop_todo.yml @@ -1,6 +1,6 @@ # This configuration was generated by # `rubocop --auto-gen-config --auto-gen-only-exclude --exclude-limit 99999` -# on 2021-08-28 18:08:22 UTC using RuboCop version 1.20.0. +# on 2023-01-20 19:16:15 UTC using RuboCop version 1.43.0. # The point is for the user to remove these configuration records # one by one as the offenses are removed from the code base. # Note that changes in the inspected code, or installation of new @@ -17,7 +17,15 @@ Betterment/ActiveJobPerformable: - 'spec/autoloaded/struct.rb' - 'spec/sample_jobs.rb' -# Offense count: 4 +# Offense count: 1 +# This cop supports safe autocorrection (--autocorrect). +# Configuration parameters: Severity, Include. +# Include: **/*.gemspec +Gemspec/DeprecatedAttributeAssignment: + Exclude: + - 'delayed.gemspec' + +# Offense count: 2 # Configuration parameters: AllowedMethods. # AllowedMethods: enums Lint/ConstantDefinitionInBlock: @@ -42,7 +50,7 @@ Lint/SuppressedException: - 'lib/delayed/backend/base.rb' # Offense count: 4 -# Configuration parameters: IgnoredMethods, CountRepeatedAttributes, Max. +# Configuration parameters: AllowedMethods, AllowedPatterns, IgnoredMethods, CountRepeatedAttributes, Max. Metrics/AbcSize: Exclude: - 'lib/delayed/message_sending.rb' @@ -54,6 +62,22 @@ RSpec/AnyInstance: Exclude: - 'spec/delayed/job_spec.rb' +# Offense count: 18 +# This cop supports unsafe autocorrection (--autocorrect-all). +RSpec/BeEq: + Exclude: + - 'spec/delayed/job_spec.rb' + - 'spec/delayed/priority_spec.rb' + - 'spec/message_sending_spec.rb' + +# Offense count: 1 +# This cop supports safe autocorrection (--autocorrect). +# Configuration parameters: EnforcedStyle. +# SupportedStyles: be_a, be_kind_of +RSpec/ClassCheck: + Exclude: + - 'spec/yaml_ext_spec.rb' + # Offense count: 2 RSpec/ExpectInHook: Exclude: @@ -67,7 +91,7 @@ RSpec/InstanceVariable: - 'spec/performable_method_spec.rb' - 'spec/worker_spec.rb' -# Offense count: 4 +# Offense count: 2 RSpec/LeakyConstantDeclaration: Exclude: - 'spec/message_sending_spec.rb' @@ -79,7 +103,7 @@ RSpec/StubbedMock: - 'spec/performable_method_spec.rb' - 'spec/worker_spec.rb' -# Offense count: 6 +# Offense count: 5 # Configuration parameters: IgnoreNameless, IgnoreSymbolicNames. RSpec/VerifiedDoubles: Exclude: @@ -89,13 +113,19 @@ RSpec/VerifiedDoubles: - 'spec/worker_spec.rb' # Offense count: 1 -# Cop supports --auto-correct. +# This cop supports unsafe autocorrection (--autocorrect-all). +Rails/ActiveSupportOnLoad: + Exclude: + - 'lib/delayed.rb' + +# Offense count: 1 +# This cop supports unsafe autocorrection (--autocorrect-all). Rails/ApplicationMailer: Exclude: - 'spec/performable_mailer_spec.rb' # Offense count: 2 -# Cop supports --auto-correct. +# This cop supports unsafe autocorrection (--autocorrect-all). # Configuration parameters: Include. # Include: **/Rakefile, **/*.rake Rails/RakeEnvironment: @@ -109,8 +139,14 @@ Rails/SkipsModelValidations: Exclude: - 'app/models/delayed/job.rb' +# Offense count: 4 +# This cop supports safe autocorrection (--autocorrect). +Rails/StripHeredoc: + Exclude: + - 'spec/psych_ext_spec.rb' + # Offense count: 2 -# Cop supports --auto-correct. +# This cop supports unsafe autocorrection (--autocorrect-all). # Configuration parameters: EnforcedStyle. # SupportedStyles: strict, flexible Rails/TimeZone: @@ -119,7 +155,7 @@ Rails/TimeZone: - 'spec/worker_spec.rb' # Offense count: 2 -# Cop supports --auto-correct. +# This cop supports safe autocorrection (--autocorrect). Rake/Desc: Exclude: - 'Rakefile' @@ -129,14 +165,27 @@ Rake/DuplicateTask: Exclude: - 'Rakefile' -# Offense count: 5 -# Cop supports --auto-correct. +# Offense count: 4 +# This cop supports unsafe autocorrection (--autocorrect-all). Security/YAMLLoad: Exclude: - 'spec/delayed/serialization/active_record_spec.rb' - 'spec/helper.rb' - 'spec/yaml_ext_spec.rb' +# Offense count: 2 +# This cop supports safe autocorrection (--autocorrect). +# Configuration parameters: AllowedVars. +Style/FetchEnvVar: + Exclude: + - 'spec/helper.rb' + +# Offense count: 1 +# This cop supports unsafe autocorrection (--autocorrect-all). +Style/MinMaxComparison: + Exclude: + - 'lib/delayed/backend/base.rb' + # Offense count: 2 Style/MissingRespondToMissing: Exclude: @@ -149,3 +198,9 @@ Style/MissingRespondToMissing: Style/OptionalBooleanParameter: Exclude: - 'lib/delayed/performable_method.rb' + +# Offense count: 3 +# This cop supports safe autocorrection (--autocorrect). +Style/RedundantConstantBase: + Exclude: + - 'spec/delayed/job_spec.rb' diff --git a/Appraisals b/Appraisals index b3662b64..7ba4df2e 100644 --- a/Appraisals +++ b/Appraisals @@ -1,12 +1,18 @@ appraise 'rails-5-2' do + gem 'actionmailer', '~> 5.2.0' + gem 'activejob', '~> 5.2.0' gem 'activerecord', '~> 5.2.0' end appraise 'rails-6-0' do + gem 'actionmailer', '~> 6.0.0' + gem 'activejob', '~> 6.0.0' gem 'activerecord', '~> 6.0.0' end appraise 'rails-6-1' do + gem 'actionmailer', '~> 6.1.0' + gem 'activejob', '~> 6.1.0' gem 'activerecord', '~> 6.1.0' end diff --git a/CHANGELOG.md b/CHANGELOG.md index 7bdfb4aa..61336ef7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,13 @@ and this project aims to adhere to [Semantic Versioning](http://semver.org/spec/ ### Removed ### Fixed +## [0.5.0] - 2023-01-20 +### Changed +- Reduced handler size by excluding redundant 'job:' key (only 'job_data:' is + necessary). This ensures that a job can be deserialized even if the underlying + ActiveJob class is unknown to the worker, and will result in a retryable + `NameError` instead of a permanently-failed `DeserializationError`. + ## [0.4.0] - 2021-11-30 ### Fixed - Fix Ruby 3.0 kwarg compatibility issue when executing jobs enqueued via the @@ -53,6 +60,7 @@ and this project aims to adhere to [Semantic Versioning](http://semver.org/spec/ ancestor repos (`delayed_job` and `delayed_job_active_record`), plus the changes from Betterment's internal forks. +[0.5.0]: https://github.com/betterment/delayed/compare/v0.4.0...v0.5.0 [0.4.0]: https://github.com/betterment/delayed/compare/v0.3.0...v0.4.0 [0.3.0]: https://github.com/betterment/delayed/compare/v0.2.0...v0.3.0 [0.2.0]: https://github.com/betterment/delayed/compare/v0.1.1...v0.2.0 diff --git a/delayed.gemspec b/delayed.gemspec index 71401473..ae7ec4b2 100644 --- a/delayed.gemspec +++ b/delayed.gemspec @@ -18,7 +18,7 @@ Gem::Specification.new do |spec| spec.require_paths = ['lib'] spec.summary = 'a multi-threaded, SQL-driven ActiveJob backend used at Betterment to process millions of background jobs per day' - spec.version = '0.4.0' + spec.version = '0.5.0' spec.metadata = { 'changelog_uri' => 'https://github.com/betterment/delayed/blob/main/CHANGELOG.md', 'bug_tracker_uri' => 'https://github.com/betterment/delayed/issues', diff --git a/gemfiles/rails_5_2.gemfile b/gemfiles/rails_5_2.gemfile index 027888dd..da403cf3 100644 --- a/gemfiles/rails_5_2.gemfile +++ b/gemfiles/rails_5_2.gemfile @@ -2,6 +2,8 @@ source "https://rubygems.org" +gem "actionmailer", "~> 5.2.0" +gem "activejob", "~> 5.2.0" gem "activerecord", "~> 5.2.0" gemspec path: "../" diff --git a/gemfiles/rails_6_0.gemfile b/gemfiles/rails_6_0.gemfile index b07bd133..2b1829e4 100644 --- a/gemfiles/rails_6_0.gemfile +++ b/gemfiles/rails_6_0.gemfile @@ -2,6 +2,8 @@ source "https://rubygems.org" +gem "actionmailer", "~> 6.0.0" +gem "activejob", "~> 6.0.0" gem "activerecord", "~> 6.0.0" gemspec path: "../" diff --git a/gemfiles/rails_6_1.gemfile b/gemfiles/rails_6_1.gemfile index 07548db0..9cc6a6af 100644 --- a/gemfiles/rails_6_1.gemfile +++ b/gemfiles/rails_6_1.gemfile @@ -2,6 +2,8 @@ source "https://rubygems.org" +gem "actionmailer", "~> 6.1.0" +gem "activejob", "~> 6.1.0" gem "activerecord", "~> 6.1.0" gemspec path: "../" diff --git a/lib/delayed/active_job_adapter.rb b/lib/delayed/active_job_adapter.rb index a7b56217..3a1d6fee 100644 --- a/lib/delayed/active_job_adapter.rb +++ b/lib/delayed/active_job_adapter.rb @@ -56,6 +56,10 @@ def perform end end + def encode_with(coder) + coder['job_data'] = @job_data + end + private def job diff --git a/spec/delayed/active_job_adapter_spec.rb b/spec/delayed/active_job_adapter_spec.rb index 31914f3f..1f2ff77f 100644 --- a/spec/delayed/active_job_adapter_spec.rb +++ b/spec/delayed/active_job_adapter_spec.rb @@ -23,6 +23,40 @@ def perform; end ActiveJob::Base.queue_adapter = adapter_was end + it 'serializes a JobWrapper in the handler with expected fields' do + Timecop.freeze('2023-01-20T18:52:29Z') do + JobClass.perform_later + end + + Delayed::Job.last.tap do |dj| + expect(dj.handler.lines).to match [ + "--- !ruby/object:Delayed::JobWrapper\n", + "job_data:\n", + " job_class: JobClass\n", + / job_id: '?#{dj.payload_object.job_id}'?\n/, + " provider_job_id: \n", + " queue_name: default\n", + " priority: \n", + " arguments: []\n", + " executions: 0\n", + (" exception_executions: {}\n" if ActiveJob::VERSION::MAJOR >= 6), + " locale: en\n", + (" timezone: \n" if ActiveJob::VERSION::MAJOR >= 6), + (" enqueued_at: '2023-01-20T18:52:29Z'\n" if ActiveJob::VERSION::MAJOR >= 6), + ].compact + end + end + + it 'deserializes even if the underlying job class is not defined' do + JobClass.perform_later + + Delayed::Job.last.tap do |dj| + dj.handler = dj.handler.gsub('JobClass', 'MissingJobClass') + expect { dj.payload_object }.not_to raise_error + expect { dj.payload_object.job_id }.to raise_error(NameError, 'uninitialized constant MissingJobClass') + end + end + describe '.set' do it 'supports priority as an integer' do JobClass.set(priority: 43).perform_later