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

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

Open
branliu0 opened this issue Oct 7, 2015 · 8 comments
Open

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

branliu0 opened this issue Oct 7, 2015 · 8 comments

Comments

@branliu0
Copy link
Contributor

branliu0 commented Oct 7, 2015

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.

@lantins
Copy link
Owner

lantins commented Nov 1, 2015

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

@branliu0
Copy link
Contributor Author

branliu0 commented Nov 2, 2015

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

@jzaleski
Copy link
Collaborator

jzaleski commented Nov 3, 2015

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?

@lantins
Copy link
Owner

lantins commented Nov 3, 2015

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'?

@jzaleski
Copy link
Collaborator

jzaleski commented Nov 4, 2015

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
#127 (comment)
.

@lantins
Copy link
Owner

lantins commented Nov 4, 2015

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

@jzaleski
Copy link
Collaborator

jzaleski commented Nov 5, 2015

Excellent. Glad we're in agreement.

@lantins
Copy link
Owner

lantins commented Nov 5, 2015

👍

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

No branches or pull requests

3 participants