Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Keyword argument support in v6.4.0 breaks for jobs enqueued before v6.4.0 #5138

Closed
valscion opened this issue Jan 24, 2022 · 3 comments
Closed

Keyword argument support in v6.4.0 breaks for jobs enqueued before v6.4.0 #5138

valscion opened this issue Jan 24, 2022 · 3 comments

Comments

@valscion
Copy link

Ruby version: 2.7.5
Rails version: 6.0.4.3
Sidekiq / Pro / Enterprise version(s): v6.4.0

It seems that this PR:

caused mails enqueued with Sidekiq versions before v6.4.0 to stop working and crash to this error:

NoMethodError: undefined method `empty?' for nil:NilClass

I assume that this is because in here:

https://github.com/mperham/sidekiq/blob/34d081f4ed54873663e70dc1e2795d8646bfe15a/lib/sidekiq/extensions/action_mailer.rb#L18-L20

the code assumes that the incoming yml parameter contains four parameters and does not protect against the last kwargs parameter not being there in the first place. kwargs is nil when the DelayedMailer job has been enqueued on a version of Sidekiq prior to v6.4.0.

I don't know if new versions of Sidekiq should make the jobs compatible with the previous version, so this might not be an issue worth of fixing. We were able to re-enqueue the one job we got stuck in the queue and remove the old one.

Initializer

The big thing in initializer is the enabling of .delay API

Sidekiq::Extensions.enable_delay!

Here's the entire initializer for completeness sake:

# frozen_string_literal: true

# Simple middleware to save the current locale and restore it when the job executes.
require 'sidekiq/middleware/i18n'

ActiveSupport::Reloader.to_prepare do
  Sidekiq.configure_client do |config|
    # Fix code reload for SiteSidekiqMiddleware::Client by removing
    # any old references we might have to that class.
    config
      .client_middleware
      .entries
      .delete_if do |middleware|
        middleware.klass.name == 'SiteSidekiqMiddleware::Client'
      end

    # Saves the current site and restore it when job executes
    config.client_middleware do |chain|
      chain.add ::SiteSidekiqMiddleware::Client
    end
  end
end

# Enable `.delay` API. It has been disabled by default.
# https://github.com/mperham/sidekiq/blob/v5.1.3/5.0-Upgrade.md
Sidekiq::Extensions.enable_delay!

Sidekiq.configure_server do |config|
  # Fix mail gem not being thread-safe, potentially causing errors when ran in Sidekiq
  # https://github.com/mikel/mail/issues/912
  # https://github.com/mperham/sidekiq/issues/3295
  #
  # Solution from https://github.com/mikel/mail/issues/912#issuecomment-214850355
  require 'mail'
  Mail.eager_autoload!

  config.client_middleware { |chain| chain.add ::SiteSidekiqMiddleware::Client }
  config.server_middleware { |chain| chain.add ::SiteSidekiqMiddleware::Server }
end

We're currently in the process of cleaning up the middleware related tomfoolery that existed before we enabled Zeitwerk. That code should be irrelevant to this issue, though.

Backtrace:

2022-01-24T13:30:04.120Z pid=5446 tid=4h1e WARN: {"context":"Job raised exception","job":{"retry":true,"queue":"default","class":"Sidekiq::Extensions::DelayedMailer","args":["---\n- !ruby/class 'Inquiries::MessageMailer'\n- :reply\n- - 1337510\n"],"jid":"48917f3cf1c2593843fcf2b1","created_at":1643030471.3704283,"locale":"fi","site_key":"venuu_fi","enqueued_at":1643031004.1158407,"error_message":"undefined method `empty?' for nil:NilClass","error_class":"NoMethodError","failed_at":1643030471.3758464,"retry_count":4,"retried_at":1643030717.118036},"jobstr":"{\"retry\":true,\"queue\":\"default\",\"class\":\"Sidekiq::Extensions::DelayedMailer\",\"args\":[\"---\\n- !ruby/class 'Inquiries::MessageMailer'\\n- :reply\\n- - 1337510\\n\"],\"jid\":\"48917f3cf1c2593843fcf2b1\",\"created_at\":1643030471.3704283,\"locale\":\"fi\",\"site_key\":\"venuu_fi\",\"enqueued_at\":1643031004.1158407,\"error_message\":\"undefined method `empty?' for nil:NilClass\",\"error_class\":\"NoMethodError\",\"failed_at\":1643030471.3758464,\"retry_count\":4,\"retried_at\":1643030717.118036}"}
2022-01-24T13:30:04.120Z pid=5446 tid=4h1e WARN: NoMethodError: undefined method `empty?' for nil:NilClass
2022-01-24T13:30:04.120Z pid=5446 tid=4h1e WARN: /var/app/vendor/bundle/ruby/2.7.0/gems/sidekiq-6.4.0/lib/sidekiq/extensions/action_mailer.rb:20:in `perform'
/var/app/vendor/bundle/ruby/2.7.0/gems/sidekiq-6.4.0/lib/sidekiq/processor.rb:196:in `execute_job'
/var/app/vendor/bundle/ruby/2.7.0/gems/sidekiq-6.4.0/lib/sidekiq/processor.rb:164:in `block (2 levels) in process'
/var/app/vendor/bundle/ruby/2.7.0/gems/sidekiq-6.4.0/lib/sidekiq/middleware/chain.rb:138:in `block in invoke'
/var/app/app/concepts/site/modules/site_sidekiq_middleware/server.rb:10:in `block in call'
/var/app/app/concepts/site/modules/site.rb:195:in `with_current'
/var/app/app/concepts/site/modules/site_sidekiq_middleware/server.rb:10:in `call'
/var/app/vendor/bundle/ruby/2.7.0/gems/sidekiq-6.4.0/lib/sidekiq/middleware/chain.rb:140:in `block in invoke'
/var/app/vendor/bundle/ruby/2.7.0/gems/i18n-1.8.11/lib/i18n.rb:314:in `with_locale'
/var/app/vendor/bundle/ruby/2.7.0/gems/sidekiq-6.4.0/lib/sidekiq/middleware/i18n.rb:22:in `call'
/var/app/vendor/bundle/ruby/2.7.0/gems/sidekiq-6.4.0/lib/sidekiq/middleware/chain.rb:140:in `block in invoke'
/var/app/vendor/bundle/ruby/2.7.0/gems/ddtrace-0.54.1/lib/ddtrace/contrib/sidekiq/server_tracer.rb:43:in `block in call'
/var/app/vendor/bundle/ruby/2.7.0/gems/ddtrace-0.54.1/lib/ddtrace/tracer.rb:283:in `trace'
/var/app/vendor/bundle/ruby/2.7.0/gems/ddtrace-0.54.1/lib/ddtrace/contrib/sidekiq/server_tracer.rb:24:in `call'
/var/app/vendor/bundle/ruby/2.7.0/gems/sidekiq-6.4.0/lib/sidekiq/middleware/chain.rb:140:in `block in invoke'
/var/app/vendor/bundle/ruby/2.7.0/gems/honeybadger-4.9.0/lib/honeybadger/plugins/sidekiq.rb:10:in `call'
/var/app/vendor/bundle/ruby/2.7.0/gems/sidekiq-6.4.0/lib/sidekiq/middleware/chain.rb:140:in `block in invoke'
/var/app/vendor/bundle/ruby/2.7.0/gems/sidekiq-6.4.0/lib/sidekiq/middleware/chain.rb:143:in `invoke'
/var/app/vendor/bundle/ruby/2.7.0/gems/sidekiq-6.4.0/lib/sidekiq/processor.rb:163:in `block in process'
/var/app/vendor/bundle/ruby/2.7.0/gems/sidekiq-6.4.0/lib/sidekiq/processor.rb:136:in `block (6 levels) in dispatch'
/var/app/vendor/bundle/ruby/2.7.0/gems/sidekiq-6.4.0/lib/sidekiq/job_retry.rb:114:in `local'
/var/app/vendor/bundle/ruby/2.7.0/gems/sidekiq-6.4.0/lib/sidekiq/processor.rb:135:in `block (5 levels) in dispatch'
/var/app/vendor/bundle/ruby/2.7.0/gems/sidekiq-6.4.0/lib/sidekiq/rails.rb:14:in `block in call'
/var/app/vendor/bundle/ruby/2.7.0/gems/activesupport-6.0.4.3/lib/active_support/execution_wrapper.rb:88:in `wrap'
/var/app/vendor/bundle/ruby/2.7.0/gems/activesupport-6.0.4.3/lib/active_support/reloader.rb:72:in `block in wrap'
/var/app/vendor/bundle/ruby/2.7.0/gems/activesupport-6.0.4.3/lib/active_support/execution_wrapper.rb:88:in `wrap'
/var/app/vendor/bundle/ruby/2.7.0/gems/activesupport-6.0.4.3/lib/active_support/reloader.rb:71:in `wrap'
/var/app/vendor/bundle/ruby/2.7.0/gems/sidekiq-6.4.0/lib/sidekiq/rails.rb:13:in `call'
/var/app/vendor/bundle/ruby/2.7.0/gems/sidekiq-6.4.0/lib/sidekiq/processor.rb:131:in `block (4 levels) in dispatch'
/var/app/vendor/bundle/ruby/2.7.0/gems/sidekiq-6.4.0/lib/sidekiq/processor.rb:257:in `stats'
/var/app/vendor/bundle/ruby/2.7.0/gems/sidekiq-6.4.0/lib/sidekiq/processor.rb:126:in `block (3 levels) in dispatch'
/var/app/vendor/bundle/ruby/2.7.0/gems/sidekiq-6.4.0/lib/sidekiq/job_logger.rb:13:in `call'
/var/app/vendor/bundle/ruby/2.7.0/gems/sidekiq-6.4.0/lib/sidekiq/processor.rb:125:in `block (2 levels) in dispatch'
/var/app/vendor/bundle/ruby/2.7.0/gems/sidekiq-6.4.0/lib/sidekiq/job_retry.rb:81:in `global'
/var/app/vendor/bundle/ruby/2.7.0/gems/sidekiq-6.4.0/lib/sidekiq/processor.rb:124:in `block in dispatch'
/var/app/vendor/bundle/ruby/2.7.0/gems/sidekiq-6.4.0/lib/sidekiq/logger.rb:11:in `with'
/var/app/vendor/bundle/ruby/2.7.0/gems/sidekiq-6.4.0/lib/sidekiq/job_logger.rb:33:in `prepare'
/var/app/vendor/bundle/ruby/2.7.0/gems/sidekiq-6.4.0/lib/sidekiq/processor.rb:123:in `dispatch'
/var/app/vendor/bundle/ruby/2.7.0/gems/sidekiq-6.4.0/lib/sidekiq/processor.rb:162:in `process'
/var/app/vendor/bundle/ruby/2.7.0/gems/sidekiq-6.4.0/lib/sidekiq/processor.rb:78:in `process_one'
/var/app/vendor/bundle/ruby/2.7.0/gems/sidekiq-6.4.0/lib/sidekiq/processor.rb:68:in `run'
/var/app/vendor/bundle/ruby/2.7.0/gems/sidekiq-6.4.0/lib/sidekiq/util.rb:56:in `watchdog'
/var/app/vendor/bundle/ruby/2.7.0/gems/sidekiq-6.4.0/lib/sidekiq/util.rb:65:in `block in safe_thread'
@mperham
Copy link
Collaborator

mperham commented Jan 24, 2022

Paging @4e4c52

@4e4c52
Copy link
Contributor

4e4c52 commented Jan 24, 2022

Fixed in #5142

mperham pushed a commit that referenced this issue Jan 24, 2022
@4e4c52
Fix an error when job was enqueued without kwargs (#5138) (#5142)
24fb5d3
@mperham mperham closed this as completed Jan 27, 2022
mperham added a commit that referenced this issue Jan 31, 2022
@mperham
Revert "Fix an error when job was enqueued without kwargs (#5138) (#5142
b561951 
)"

This reverts commit 24fb5d3.
@fedetaglia fedetaglia mentioned this issue Feb 1, 2022
Closed
@nevinera
Copy link

nevinera commented Feb 4, 2022 •
edited

Any word on when a 6.4.1 release might land? 6.4.0 resolves a (minor, for us) CVE, but we can't safely adopt it without this change :-\

Edit: Never mind, just landed :-)

@yob yob mentioned this issue Feb 8, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@mperham @4e4c52 @nevinera @valscion