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.

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

Fix Sidekiq setting non-serializable Class as resource #947

Merged
merged 2 commits into from
Feb 26, 2020

Conversation

delner
Copy link
Contributor

@delner delner commented Feb 13, 2020

Fixes #943

In Rails ActiveJob via rails/rails@0e64348 they pass wrapped as a Class constant instead of a String. When this constant is set as the resource, it's suspected that it cannot be converted via Msgpack and raises an error on serialization.

This pull request makes sure the job resource is always a string.

@delner delner added bug Involves a bug integrations Involves tracing integrations community Was opened by a community member labels Feb 13, 2020
@delner delner requested review from marcotc and a team February 13, 2020 19:52
@delner delner self-assigned this Feb 13, 2020
@delner
Copy link
Contributor Author

delner commented Feb 13, 2020

I haven't been able to use tests to recreate the original bug, so the current implementation is a bit of a shot in the dark.

Whenever I get job in the instrumentation, it appears all the keys have already been serialized to strings.... I'm not sure if this is a side effect of using Sidekiq::Testing.inline! or not.

{"retry"=>true,
 "queue"=>"default",
 "class"=>"ActiveJob::QueueAdapters::SidekiqAdapter::JobWrapper",
 "wrapped"=>"EmptyJob",
 "args"=>
  [{"job_class"=>"EmptyJob",
    "job_id"=>"72cf8cb1-f9e4-4d3c-94d7-7a90ed69d987",
    "provider_job_id"=>nil,
    "queue_name"=>"default",
    "priority"=>nil,
    "arguments"=>[],
    "executions"=>0,
    "exception_executions"=>{},
    "locale"=>"en",
    "timezone"=>"UTC",
    "enqueued_at"=>"2020-02-13T19:43:22Z"}],
 "jid"=>"ac04eb720a1771775a72dca7",
 "created_at"=>1581623002.2207384,
 "id"=>"4edf20b05f11e86a08d4b3d0"}

@pj0tr can you confirm this fix actually works?

Copy link
Member

@marcotc marcotc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit: !

@delner
Copy link
Contributor Author

delner commented Feb 26, 2020

Reported as working via #943 (comment) so I think this is good to merge.

@delner delner merged commit d189c04 into master Feb 26, 2020
@delner delner deleted the fix/sidekiq_with_rails_6.0.2 branch February 26, 2020 17:22
@delner delner added this to the 0.33.0 milestone Mar 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Involves a bug community Was opened by a community member integrations Involves tracing integrations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sidekiq integration broke with Rails 6.0.2
2 participants