Skip to content

Avoid exception when db-connection is lost #717

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

Avoid exception when db-connection is lost #717

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion lib/active_record/connection_adapters/sqlserver/database_statements.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -281,7 +281,8 @@ def sp_executesql_sql(sql, types, params, name)
def raw_connection_do(sql)
case @connection_options[:mode]
when :dblib
@connection.execute(sql).do
result = @connection.execute(sql)
result == false ? false : result.do
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this is essentially the same thing, but

result && result.do

might be more concise.

I did not review the functionality of the change, though.
I am just telling you an idiomatic ruby trick.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, it's not the same.

result && result.do

will return result if it is any falsy value.
When you look at the tiny_tds lib, you will see that it should either returns false or a result object which then can be called do on it.
In any other case - for example nil - I don't want it to return nil, but cause an exception, so it's easier to figure out what is going wrong.

end
ensure
@update_sql = false
Expand Down
3 changes: 1 addition & 2 deletions lib/active_record/connection_adapters/sqlserver_adapter.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,8 +152,7 @@ def disable_referential_integrity

def active?
return false unless @connection
raw_connection_do 'SELECT 1'
true
raw_connection_do('SELECT 1') == 1
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO the previous one before the change is more robust.
Because the return value is independent of raw_connection_do implementation.

No matter how raw_connection_do changes in the future, as long as still raise expected exceptions.
This function will still work correctly.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem with the previous version is that if raw_connection do does not return 1, but for example false, like it currently does when the connection is gone, no exception is raised which caused active? to return true, although the connection is not working.
With this change it will now only return true if the connection actually works, and false if an exception is raised, or anything else causes the connection to not return proper values any more.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additionally I'd like to point out that I did not get rid of the rescue block, so the previous behaviour of returning false if an connection_errors is raised is still the same.

rescue *connection_errors
false
end
Expand Down