TopLevel::Namespace.send_later(:foo) no longer works #41

Closed
mguterl opened this Issue Feb 11, 2010 · 10 comments

Comments

Projects
None yet
3 participants

mguterl commented Feb 11, 2010

I'm not sure why, but it seems like when I use send_later on classes that are located under a module, DJ is not properly enqueuing them.

This is with HEAD, I'll be investigating as this is a major issue for us.

This commit is likely the culprit: 35a2808

mguterl commented Feb 11, 2010

That's definitely the commit.

Legacy::EmployerFeed.send_later(:process)

results in a warning in the log:
Could not load object for job: Couldn't find Legacy::EmployerFeed without an ID

The problem is, that this job does not belong to a specific ActiveRecord instance, but it is a class method on Legacy::EmployerFeed.

I'm trying to debug it, but I'm very confused about your extensions to Class at the top of the file.

class Class
  def load_for_delayed_job(arg)
    self
  end

  def dump_for_delayed_job
    name
  end
end

Class#load_for_delayed_job is just throwing away arg, which in my case is nil.

def load(obj)
  if STRING_FORMAT === obj
    $1.constantize.load_for_delayed_job($2)
  else
    obj
  end
rescue => e
  Delayed::Worker.logger.warn "Could not load object for job: #{e.message}"
  raise PerformableMethod::LoadError
end

mguterl commented Feb 11, 2010

I haven't been able to fix the issue, but I can at least provide a failing test case. :)

http://github.com/recruitmilitary/delayed_job/tree/41-namespaced_class_methods_on_active_record_models

mguterl commented Feb 11, 2010

Something about this that is also somewhat problematic for our case, is that we lost all of jobs that were structured this way. I believe that if Delayed::PerformableMethod#perform fails, it should not be rescued, but allowed to bubble up and be stored in the queue as a failed job. Simply returning true if an exception is rescued is a bad idea.

Allow class methods to be queued on ActiveRecord objects. Closed by 782ceb8

Issue is fixed.

I agree that in this case, raising the error would have helped catch this bug.

But the idea of rescuing the error is that if an active record object is deleted, the job will never be able to be run. I can't think of a case where I would care that the job for a deleted record can't be run. You could probably convince me otherwise with a strong use case, but I can't think of one right now.

mguterl commented Feb 11, 2010

What about just rescuing ActiveRecord::RecordNotFound ?

def load(obj)
  if STRING_FORMAT === obj
    $1.constantize.load_for_delayed_job($2)
  else
    obj
  end
rescue ActiveRecord::RecordNotFound => e
  Delayed::Worker.logger.warn "Could not load object for job: #{e.message}"
  raise PerformableMethod::LoadError
end

That's how it used to be structured, but now that we're supporting multiple backends, I can't do it there without breaking the ability for people to use their own backends. I could modify the ActiveRecord::Base.load_for_delayed_job method to rescue NotFound and raise Delayed::LoadError

mguterl commented Feb 11, 2010

That sounds reasonable, I forgot you're adding support for MongoMapper. I agree that if a record was deleted, it shouldn't raise an error. However, I think most other errors should be recorded in a recoverable way. But then again, I'm speaking mostly from my experiences this morning in dealing with lost jobs.

Thanks so much for all the awesome work you've done with DJ and I really appreciate your responsiveness this morning.

cloudkj commented Jun 24, 2010

I can't think of a case where I would care that the job for a deleted record can't be run.

What about cases where the delayed job actually tries to load a record before it's created, say as a result of an after_create callback? I've seeing instances where the delayed job runs before the record is actually stored.

However, this behavior is actually documented in the ActiveRecord::Callbacks docs for after_create, so I guess the model/observer should take on the responsibility of handling this.

This issue was closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment