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

Incorrect behaviour in DelayedJob #291

Closed
ka8725 opened this issue Sep 7, 2015 · 6 comments
Closed

Incorrect behaviour in DelayedJob #291

ka8725 opened this issue Sep 7, 2015 · 6 comments

Comments

@ka8725
Copy link

ka8725 commented Sep 7, 2015

I recently noticed that Rollbar doesn't work in my DelayedJob tasks. Tried to debug: just placed raise 'h' in the delayed method and run Delayed::Job.last.invoke_job from rails console. This is the result:

[Rollbar] Scheduling payload
[Rollbar] Sending payload
[Rollbar] Got unexpected status code from Rollbar api: 422
[Rollbar] Response: {
  "err": 1,
  "message": "Invalid format. data.request string value found, but a object is required. data.request an object is required."
}
[Rollbar] Details: https://rollbar.com/instance/uuid?uuid=a351fb56-2ee9-4628-88d6-d7a735051771 (only available if report was successful)
RuntimeError: h

Any ideas what is wrong?

@ka8725
Copy link
Author

ka8725 commented Sep 7, 2015

More info about this bug: it works with report_dj_data = false option set.

@ri4a
Copy link

ri4a commented Sep 8, 2015

Pull request #292 fixes this.

@brianr
Copy link
Member

brianr commented Sep 8, 2015

Thanks @ka8725 and @ri4a!

What version of delayed_job were you seeing this in? I am fairly confident that this has worked in the past, so I'm guessing it may be broken in the latest version / some other version than we are already testing in. We'll want to update the test suite to make sure it covers this.

@ka8725
Copy link
Author

ka8725 commented Sep 8, 2015

I use delayed_job_active_record-4.0.3 Ruby 2.1.5

@ri4a
Copy link

ri4a commented Sep 8, 2015

It doesn't look like a delayed_job issue. Last time we received a delayed_job error was right before we upgraded from 1.5.2. to 2.1.1...

Looks like the problem was introduced when the enforce_valid_utf8 method in /lib/rollbar.rb was changed from

def enforce_valid_utf8(payload)
  normalizer = lambda do |object|
    is_symbol = object.is_a?(Symbol)
    return object unless object == object.to_s || is_symbol
    # ...removed some code here...
  end
  Rollbar::Util.iterate_and_update(payload, normalizer)
end

to

def enforce_valid_utf8(payload)
  normalizer = lambda { |object| Encoding.encode(object) }
  Rollbar::Util.iterate_and_update(payload, normalizer)
end

Note the

return object unless object == object.to_s

in the old method, whereas in the new one the object is immediately converted to String:

def encode
  value = object.to_s
  encoding = value.encoding

  # This will be most of cases so avoid force anything for them
  if encoding == ::Encoding::UTF_8 && value.valid_encoding?
    encoded_value = value
  else
    encoded_value = force_encoding(value).encode(*encoding_args(value))
  end

  object.is_a?(Symbol) ? encoded_value.to_sym : encoded_value
end

#292 ensures data.request is already the right JSON String instead of it becoming something like "#Delayed::Backend::ActiveRecord::Job:0xf474fc8" when UTF8 encoding is enforced.

jondeandres added a commit that referenced this issue Sep 10, 2015
Since serialization is made by JSON, we crashed delayed job reports
cause we were exploting the JSON & ActiveSupport bug. Now we get the job
data from job.payload_object.as_json.

I've added tests for this, testing two different scenarios:

- method without arguments
- method with arguments
jondeandres added a commit that referenced this issue Sep 11, 2015
Since serialization is made by JSON, we crashed delayed job reports
cause we were exploting the JSON & ActiveSupport bug. Now we get the job
data from job.payload_object.as_json.

I've added tests for this, testing two different scenarios:

- method without arguments
- method with arguments

Delete delayed_jobs before each spec.

Use job.as_json for delayed_job.
@jondeandres
Copy link
Contributor

@ka8725 @ri4a I have this PR and branch prepared, #293. I've tested it and it should work properly. We'll merge it and release soon, but maybe you want to start using it in your projects and check it works as you expect.

Thanks.

jondeandres added a commit that referenced this issue Sep 15, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants