Skip to content

Commit

Permalink
Reduce handler size in some ActiveJob cases, and fix deserialization …
Browse files Browse the repository at this point in the history
…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.
  • Loading branch information
smudge authored Jan 20, 2023
1 parent f7a27c8 commit 66125d1
Show file tree
Hide file tree
Showing 9 changed files with 125 additions and 12 deletions.
77 changes: 66 additions & 11 deletions .rubocop_todo.yml
Original file line number Diff line number Diff line change
@@ -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
Expand All @@ -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:
Expand All @@ -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'
Expand All @@ -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:
Expand All @@ -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'
Expand All @@ -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:
Expand All @@ -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:
Expand All @@ -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:
Expand All @@ -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'
Expand All @@ -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:
Expand All @@ -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'
6 changes: 6 additions & 0 deletions Appraisals
Original file line number Diff line number Diff line change
@@ -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

Expand Down
8 changes: 8 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,13 @@ and this project aims to adhere to [Semantic Versioning](http://semver.org/spec/
### Removed <!-- for now removed features. -->
### Fixed <!-- for any bug fixes. -->

## [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
Expand Down Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion delayed.gemspec
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand Down
2 changes: 2 additions & 0 deletions gemfiles/rails_5_2.gemfile
Original file line number Diff line number Diff line change
Expand Up @@ -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: "../"
2 changes: 2 additions & 0 deletions gemfiles/rails_6_0.gemfile
Original file line number Diff line number Diff line change
Expand Up @@ -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: "../"
2 changes: 2 additions & 0 deletions gemfiles/rails_6_1.gemfile
Original file line number Diff line number Diff line change
Expand Up @@ -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: "../"
4 changes: 4 additions & 0 deletions lib/delayed/active_job_adapter.rb
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,10 @@ def perform
end
end

def encode_with(coder)
coder['job_data'] = @job_data
end

private

def job
Expand Down
34 changes: 34 additions & 0 deletions spec/delayed/active_job_adapter_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down

0 comments on commit 66125d1

Please sign in to comment.