-
Notifications
You must be signed in to change notification settings - Fork 507
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
SQL timeout is returning a 500 error #2497
Conversation
Added a missing check for a SqlException that occurs due to a timeout
Revised projects so that the only change necessary is just for the dependent Microsoft.Health.Fhir.Api.csproj rather than the 3 other projects.
@@ -26,6 +26,7 @@ | |||
<PackageReference Include="Microsoft.Health.Abstractions" Version="$(HealthcareSharedPackageVersion)" /> | |||
<PackageReference Include="Microsoft.Health.Api" Version="$(HealthcareSharedPackageVersion)" /> | |||
<PackageReference Include="Microsoft.Health.Extensions.DependencyInjection" Version="$(HealthcareSharedPackageVersion)" /> | |||
<PackageReference Include="Microsoft.Health.SqlServer" Version="$(HealthcareSharedPackageVersion)" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@feordin @v-mserdar we should be really careful about the dependencies we introduce between projects. I ideally don't see why the API layer should know anything about SQL.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@brendankowitz There is another way we can check for this and that is to replace the SqlException catch and replace it with a Win32Exception as that is the inner exception type. If we do that then we can remove the dependency of concern. If we go this route then the code snippet would check like the following. Please advise on how you want to handle the hardcoded 258 which is the error number that is returned. I take it we will add it to a file as a const?
else if (context.Exception is Win32Exception win32Exception && win32Exception.NativeErrorCode == 258)
{
context.Result = CreateOperationOutcomeResult(win32Exception.Message, OperationOutcome.IssueSeverity.Error, OperationOutcome.IssueType.Timeout, HttpStatusCode.RequestTimeout);
context.ExceptionHandled = true;
}
Description
SQL timeout is returning a 500 error
Related issues
Addresses 86829
Testing
Testing was manually done since I had to inject a SQL script into the stored procedure being tested. For example, during local debugging I set a breakpoint at a point where the SQLCommand.CommandText was being set, then I revised that text by adding in this specific SQL, "WAITFOR DELAY '00:00:35';". The reason for that is the default SQLCommand.CommandTimeout is 30 seconds and given the script that I added, it will force the current SQL statement to take an additional 35 seconds thereby causing a SQL timeout.
FHIR Team Checklist