Removes nil guard that causes silent failures #344

Closed
wants to merge 3 commits into
from

Projects

None yet

3 participants

@dcuddeback

The nil guard in PerformableMethod#perform means that no error is raised when attempting to delay a method on nil:

@model = Model.some_lookup_method(params) # => nil
@model.delay.process_something

In this example, if @model is nil, then PerformableMethod happily ignores the error. It seems like this was by design, because you have a spec for this exact scenario:

context "with the persisted record cannot be found" do
  before do
    @method.object = nil 
  end 

  it "should be a no-op if object is nil" do
    lambda { @method.perform }.should_not raise_error
  end 
end 

What was the rationale for this design decision? In our particular application, we've found this to be detrimental, because we don't receive failure notices when delaying a method on nil.

If you'd like to change the behavior to raise an error instead, I've attached a patch that will do that. If not, it's an easy monkey patch for others to apply. Another alternative is that we can make it configurable: Delayed::Worker.raise_error_on_nil = true.

@betamatt
Collaborator

The persisted-record condition is slightly different. That's the case where a method on a model has been delayed but the record is deleted before the job fires. We decided the best thing to do there was quietly ignore the error.

As far as delaying methods on a nil object, I agree that it's nonsensical and exceptional. I'm not sure why the decision was made to noop. It seems likely that it was to prevent accidentally delaying methods that would be accidentally valid on nil but it seems better to check for nil and raise.

@dcuddeback

@betamatt Perhaps the reason for the noop on nil is precisely the persisted-record condition that you mentioned. The example I gave in my opening comment was an example where @model is nil at the time of the call to delay, instead of being nil at the time of execution because of the model being destroyed in the intermediate time.

In our application, the code that causes this issue is a bit more complex than looking up a non-existing model, but it was a bug in our code attempting to call delay on nil. Unfortunately, the combination of having delay defined on NilClass and having the nil guard in place causes the bug (in our code) to be silently ignored when we would have preferred some notification.

I don't have strong feelings either way about quietly ignoring errors in the case where a model is deleted after a method has been delayed. Instead, how about raising the error within the call to delay if it's called on nil? That provides more immediate feedback about the error, probably being picked up by an application's error notification system (such as Airbrake). It also lets you defer any decisions about what to do about the persisted-record condition. If that sounds like a more acceptable solution to you, I'd be happy to send a new patch.

@betamatt
Collaborator
betamatt commented Apr 2, 2012

@dcuddeback That sounds fine to me. Please submit.

@bryckbost do you have strong feelings about this? I'm looking to accept a patch to raise when .delay is called on nil.

@bryckbost
Member

Sounds good to me as I don't have any other insight into why the original decision was made.

@dcuddeback

@betamatt @bryckbost I updated the code to do the nil check at the time of the delay call rather than in PerformableMethod#perform. However, while doing this, I noticed that PerformableMethod#initialize already checks if the object responds to the method. So, while nil.delay.to_s will pass the check in PerformableMethod, something like nil.delay.calculate_statistics will already raise a NoMethodError. So now I'm second-guessing the necessity of this patch. Unfortunately, I don't recall the exact circumstances around the original problem that I encountered.

@betamatt
Collaborator
betamatt commented Apr 3, 2012

Doesn't seem like there's any action to take here. Closing.

@betamatt betamatt closed this Apr 3, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment