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

unique_args method suppresses all NameError exceptions #219

Closed
percyhanna opened this issue Apr 27, 2017 · 2 comments
Closed

unique_args method suppresses all NameError exceptions #219

percyhanna opened this issue Apr 27, 2017 · 2 comments

Comments

@percyhanna
Copy link

We recently discovered that we had workers raising NoMethodError exceptions because we weren't properly handling/validating input parameters. However, these errors were unknowingly being silenced, and would simply continue on as if nothing bad had happened at all.

Rescuing all NameError exceptions is overly aggressive. My assumption is that the rescue block can be removed entirely? It seems there are already guards agains the method being undefined.

@mhenrixon
Copy link
Owner

I agree that hiding the issue is really bad. There should definitely be some logging there, the problem is that in some situations the worker class can't be instantiated because it isn't part of the codebase where we are pushing from. It might only be part of the server. This means that it was crashing for those same people when the client was checking for uniqueness.

I can't really have the unique jobs crash here. Would sufficient log output be sufficient for your situation? I could potentially add a configuration option for raising all errors as some specific gem exception and let you deal with it that way. Turning of catching the name error will cause problem for other people.

Alternatively maybe it makes sense to catch the NoMethodError and re raise it. I didn't know NoMethodError was a kind of NameError. Thought NameError's where only for when a class couldn't be found.

Learning new things every day.

@percyhanna
Copy link
Author

For one, the rescue doesn't even seem to be in the right place. Aside from that, this code implies to me that the error would never raise anyway. I'm not sure exactly under what situation the error(s) that were being experienced were happening, but the main problem is that the rescue is too broad. If @worker_class is a String because the constant is not defined, then it likely won't respond to unique_args_method, so I'm not sure what the problem is?

Is there a test case written for this specific situation? I wonder if it still needs the rescue inside unique_args, since there's also a rescue NameError in worker_class_constantize.

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

2 participants