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

DelayedJob integration should not call as_json on the payload object. #369

Closed
fcheung opened this issue Feb 1, 2016 · 12 comments
Closed

Comments

@fcheung
Copy link

fcheung commented Feb 1, 2016

The DelayedJob integration loads the payload object and tries to serialise it using as_json. If Delayed job has been used to delay a class method on an ActiveRecord subclass, this can have very unpredictable results, as one of the objects that will have as_json called on it is the class itself.

ActiveSupport adds Object#as_json which iterates over all instance variables and calls as_json - unless classes implement their own as_json method it will recursively serialise all the instance variables of the class and the class instance variables of an active record subclass contains all sorts of stuff.

It can very easily lead to infinite recursion - in my case this happened via the use of aasm gem: it creates a class instance variable containing a state machine definition object, which itself has as an instance variable the class it was defined on.

Another bad thing is that active record objects appear to have a relation object containing the base scope for the model. Calling as_json on this will attempt to dump the entire table to json

@jondeandres
Copy link
Contributor

@fcheung have you tried using Oj as JSON serializer? It's the gem we suggest to use.

We've had historiclal problems with to_json and as_json and we didn't find a solution to work on every project. Can you give us more details about the libraries you have installed?

Thanks.

@fcheung
Copy link
Author

fcheung commented Feb 1, 2016

I can reproduce the issue with this snippet (running against rails 4.2.5.1)

require 'active_record'
ActiveRecord::Base.establish_connection(adapter: 'sqlite3', database: 'test.sqlite3')
ActiveRecord::Schema.define(version: 1) do
  create_table  :users do
  end
end

class User < ActiveRecord::Base
end

User.create!
User.as_json

This produces a SystemStackError whether Oj is loaded or not. Since the call to as_json happens inside the rollbar provided delayed job integration, it's not something that I can avoid (other than by monkey patching the gem)

@fcheung
Copy link
Author

fcheung commented Feb 1, 2016

This snippet shows what happens when rollbar is involved:

require 'active_record'
require 'delayed_job'
require 'delayed_job_active_record'
require 'oj'
require 'rollbar'

Rollbar.configure do |config|
  # Without configuration, Rollbar is enabled in all environments.
  # To disable in specific environments, set config.enabled=false.

  config.access_token = ENV['TOKEN']
end

ActiveRecord::Base.establish_connection(adapter: 'sqlite3', database: 'test.sqlite3')

ActiveRecord::Schema.define(version: 1) do
  create_table  :users do
  end
  create_table :delayed_jobs, force: true do |table|
    table.integer :priority, default: 0, null: false # Allows some jobs to jump to the front of the queue
    table.integer :attempts, default: 0, null: false # Provides for retries, but still fail eventually.
    table.text :handler,                 null: false # YAML-encoded string of the object that will do work
    table.text :last_error                           # reason for last failure (See Note below)
    table.datetime :run_at                           # When to run. Could be Time.zone.now for immediately, or sometime in the future.
    table.datetime :locked_at                        # Set when a client is working on this object
    table.datetime :failed_at                        # Set when all retries have failed (actually, by default, the record is deleted instead)
    table.string :locked_by                          # Who is working on this object (if locked)
    table.string :queue                              # The name of the queue this job is in
    table.timestamps null: true
  end
end

class User < ActiveRecord::Base
  def self.foo
    raise 'error'
  end
end


User.create!
User.delay.foo
Delayed::Job.last.invoke_job

The call to invoke_job raises SystemStackError, because of the call by rollbar to User.as_json

@jondeandres
Copy link
Contributor

@fcheung thank you very much for the analysis. We'll try to reproduce the problem and fix it.

@jondeandres
Copy link
Contributor

@fcheung can you for now use this config options? config.report_dj_data = false.

We'll work on fix this soon. Thanks.

@fcheung
Copy link
Author

fcheung commented Feb 4, 2016

I've just monkey patched rollbar for now - the remainder of the data is still good to have

@jondeandres
Copy link
Contributor

@fcheung nice, can you share what you coded for that? You just remove the data you don't want?

@fcheung
Copy link
Author

fcheung commented Feb 4, 2016

It's a bodge - i just commented out https://github.com/rollbar/rollbar-gem/blob/master/lib/rollbar/delayed_job.rb#L20

job is an active record object (at least with the backend i use) so has a sane as_json method. handler ends up being a blob of yaml rather than a nice payload object but it's still pretty readable in general - for us it's never much more than a class name, method name & arguments (we tend to keep arguments to delayed jobs as just plain scalars/arrays/hashes rather than complex types)

@fcheung
Copy link
Author

fcheung commented Feb 4, 2016

module Rollbar
  module Delayed
    class JobData
      def to_hash
        job_data = job.as_json
        job_data
      end
    end
  end
end

@jondeandres
Copy link
Contributor

I see, I think I was using that solution but I found some problems in some scenarios.

I'll check it, thanks! 😄

@trostli
Copy link

trostli commented Mar 26, 2016

Just recently ran into this issue myself and am getting Stack Level too deep errors in my delayed_job workers when using Rollbar in them. The OJ gem does not seem to help.

@jondeandres
Copy link
Contributor

@trostli we'll need to review this. The bug is in ActiveSupport since it monkey patches Object. We need to find a correct way to send this data. We hope we can fix this soon.

Sorry for the inconveniences.

jondeandres added a commit that referenced this issue May 6, 2016
This will avoid infinite loops with ActiveRecord class jobs.

Fix #369
jondeandres added a commit that referenced this issue May 7, 2016
This will avoid infinite loops with ActiveRecord class jobs.

Fix #369
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

3 participants