resque-retry icon indicating copy to clipboard operation
resque-retry copied to clipboard

Callbacks don't run if they don't have the right arity

Open branliu0 opened this issue 9 years ago • 8 comments

If the arity of the callback is not right, then an ArgumentError: wrong number of arguments gets thrown, but instance_exec swallows all exceptions so basically the effect is that the callback doesn't get run.

branliu0 avatar Oct 07 '15 18:10 branliu0

Are you able to look at this issue before we look at #129

lantins avatar Nov 01 '15 22:11 lantins

Sure thing. So what do you think would be a good solution? Here are some thoughts:

  1. We could do an arity check when the callback is registered, and throw an exception if the arity is 0, since that would probably catch most cases. I think it gets a bit more complicated if we want to have a tighter guarantee on the correct arity (by matching the arity of self.perform with the callback)
  2. We can print a warning for all the exceptions we get, instead of just silently suppressing them

branliu0 avatar Nov 02 '15 10:11 branliu0

I think that the second approach might actually be favorable. Unless of course there is a reliable/easy way to do the arity check up front, during the registration process. If we dump something into the Resque log it will likely give enough context to the person{,s} trouble-shooting so that they can remedy the issue on their side, we can, and should, only guard against so much. What do you think @lantins?

jzaleski avatar Nov 03 '15 23:11 jzaleski

We need to let things blow up, as early as possible by letting the exceptions bubble through.

I guess the question is, are callbacks 'best effort' or 'job fails if callback fails'?

lantins avatar Nov 03 '15 23:11 lantins

My concern here was allowing callbacks to blow up within the internals of resque-retry. I am all for blowin up early if we can do it with 100% confidence. Perhaps we can enforce the call signature for these rather than letting them be varargs? If neither seems feasible I think the best solution here is to make it a best attempt and log when there is a failure. On Nov 3, 2015 6:24 PM, "Luke Antins" [email protected] wrote:

We need to let things blow up, as early as possible by letting the exceptions bubble through.

I guess the question is, are callbacks 'best effort' or 'job fails if callback fails'?

— Reply to this email directly or view it on GitHub https://github.com/lantins/resque-retry/issues/127#issuecomment-153521621 .

jzaleski avatar Nov 04 '15 13:11 jzaleski

Best attempts to log the failure is a good place to start.

lantins avatar Nov 04 '15 21:11 lantins

Excellent. Glad we're in agreement.

jzaleski avatar Nov 05 '15 00:11 jzaleski

:+1:

lantins avatar Nov 05 '15 09:11 lantins