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

Avoid unnecessary catching of Exception instead of StandardError (conver... #12636

Merged
merged 1 commit into from
Oct 24, 2013

Conversation

stopdropandrew
Copy link
Contributor

...ting Exceptions into StandardErrors)

Very similar to c989160, but I limited the scope to places where Exception was rescued and then re-raised as a StandardError.

@rafaelfranca
Copy link
Member

What is the motivation?

@stopdropandrew
Copy link
Contributor Author

For me, when sending a SIGTERM or SIGINT while a template is rendering, the process is rescuing the signal and not exiting cleanly.

Rescuing Exception to handle some exit logic (ActiveRecord transaction rollbacks) and the re-raising the original error is fine. But in these cases, an Exception could be rescued and then the code is raising a StandardError, essentially lowering the importance of the error.

Here's some additional info from a similar pull request - #6759

@rafaelfranca
Copy link
Member

I'm fine with the changes on the template rendering but I don't think we should change the parameters parser code. If a user submit some data that cause an Exception subclass error it will not handled and the application will die, causing a deny of service.

@stopdropandrew
Copy link
Contributor Author

That works for me, I removed the param parsing part, the template rendering is where I was actually noticing the issue.

If I start seeing it come up elsewhere I can be explicit about re-raising specific Exception subclasses.

rafaelfranca added a commit that referenced this pull request Oct 24, 2013
Avoid unnecessary catching of Exception instead of StandardError (conver...
@rafaelfranca rafaelfranca merged commit a53f716 into rails:master Oct 24, 2013
rafaelfranca added a commit that referenced this pull request Oct 24, 2013
Avoid unnecessary catching of Exception instead of StandardError (conver...
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

Successfully merging this pull request may close these issues.

None yet

2 participants