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

DatabaseFacade.CanConnect throws instead of returning false #18355

Closed
roji opened this issue Oct 13, 2019 · 8 comments · Fixed by #18867
Closed

DatabaseFacade.CanConnect throws instead of returning false #18355

roji opened this issue Oct 13, 2019 · 8 comments · Fixed by #18867
Labels
closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. type-bug
Milestone

Comments

@roji
Copy link
Member

roji commented Oct 13, 2019

On SQL Server, if the database server is down, an SqlException is thrown instead of returning false. It seems like we should catch all exceptions internally and return false instead.

@roji
Copy link
Member Author

roji commented Oct 15, 2019

@rynowak what would be the expected behavior here from the ASP.NET health check perspective? It seems problematic for this method to sometimes throw and sometimes return false - what do other health checks do?

@roji
Copy link
Member Author

roji commented Oct 23, 2019

@rynowak ping

@ajcvickers
Copy link
Member

@rynowak Pinging you now you've been right-side-up for a while. What is the general guidance on health checks and exceptions. Should any error report back as false, or should unexpected errors be surfaced as exceptions?

@rynowak
Copy link
Member

rynowak commented Nov 11, 2019

I would expect it to return false 😁

Health checks are an abstraction/isolation layer. A health check should return "Unhealthy" if the check fails, that's preferrable to an exception. If the check throws an exception that will be logged differently.

Since checks are are abstraction layer, it's also reasonable for a check to handle exceptions from whatever it's checking. Some of them do that because that's the only option. In the case that you want to have the exception convey something meaningful and log that too, it's an option.

In general though returning false is preferrable to throwing.

@ajcvickers
Copy link
Member

@rynowak Thanks!

roji added a commit that referenced this issue Nov 12, 2019
roji added a commit that referenced this issue Nov 14, 2019
@ajcvickers ajcvickers added this to the 5.0.0 milestone Nov 17, 2019
@ajcvickers ajcvickers added the closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. label Nov 17, 2019
@antoinecfmws
Copy link

Hello @roji , we are using EF Core Dbcontext 3.1.0, and the dbContext.Database.CanConnect() still throws an exception if the server is offline. I thought this was pushed to master. Thoughts?

@roji
Copy link
Member Author

roji commented Feb 14, 2020

@antoinecfmws this was done too late for 3.1.0, so will be released for 5.0.0 (and its previews). This is why this issue is in the 5.0.0 milestone.

@antoinecfmws
Copy link

Makes sense now, thank you! Sorry to bother @roji

@ajcvickers ajcvickers modified the milestones: 5.0.0, 5.0.0-preview1 Mar 13, 2020
@ajcvickers ajcvickers modified the milestones: 5.0.0-preview1, 5.0.0 Nov 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. type-bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants