From 62aa9b78bdeac342072fe140ac15d5892d767863 Mon Sep 17 00:00:00 2001 From: Will Sugarman Date: Fri, 7 Jul 2023 10:32:14 -0700 Subject: [PATCH 1/3] Refactor cancellation handling --- .../Features/Store/StoreService.cs | 54 ++++++++----------- 1 file changed, 22 insertions(+), 32 deletions(-) diff --git a/src/Microsoft.Health.Dicom.Core/Features/Store/StoreService.cs b/src/Microsoft.Health.Dicom.Core/Features/Store/StoreService.cs index c570094c0c..1b0d2cd864 100644 --- a/src/Microsoft.Health.Dicom.Core/Features/Store/StoreService.cs +++ b/src/Microsoft.Health.Dicom.Core/Features/Store/StoreService.cs @@ -189,24 +189,18 @@ public async Task ProcessAsync( } } } + catch (OperationCanceledException) + { + throw; + } catch (Exception ex) { - ushort failureCode = FailureReasonCodes.ProcessingFailure; - - switch (ex) + ushort failureCode = ex switch { - case DicomValidationException _: - failureCode = FailureReasonCodes.ValidationFailure; - break; - - case DatasetValidationException dicomDatasetValidationException: - failureCode = dicomDatasetValidationException.FailureCode; - break; - - case ValidationException _: - failureCode = FailureReasonCodes.ValidationFailure; - break; - } + DatasetValidationException dve => dve.FailureCode, + DicomValidationException or ValidationException => FailureReasonCodes.ValidationFailure, + _ => FailureReasonCodes.ProcessingFailure, + }; LogValidationFailedDelegate(_logger, index, failureCode, ex); @@ -231,26 +225,22 @@ public async Task ProcessAsync( ); return length; } + catch (ConditionalExternalException cee) when (cee.IsExternal) + { + throw; + } + catch (DataStoreException dse) when (dse.InnerException is TaskCanceledException) + { + throw; + } catch (Exception ex) { - ushort failureCode = FailureReasonCodes.ProcessingFailure; - - switch (ex) + ushort failureCode = ex switch { - case DataStoreException { IsExternal: true }: - throw; - - case DataStoreRequestFailedException { IsExternal: true }: - throw; - - case PendingInstanceException _: - failureCode = FailureReasonCodes.PendingSopInstance; - break; - - case InstanceAlreadyExistsException _: - failureCode = FailureReasonCodes.SopInstanceAlreadyExists; - break; - } + PendingInstanceException => FailureReasonCodes.PendingSopInstance, + InstanceAlreadyExistsException => FailureReasonCodes.SopInstanceAlreadyExists, + _ => FailureReasonCodes.ProcessingFailure, + }; LogFailedToStoreDelegate(_logger, index, failureCode, ex); From 67ed2272067d28210a6d520aa19bbc41d9b7d6e5 Mon Sep 17 00:00:00 2001 From: Will Sugarman Date: Fri, 7 Jul 2023 12:40:58 -0700 Subject: [PATCH 2/3] Add tests --- .../Features/Store/DicomStoreServiceTests.cs | 43 +++++++++++++++++++ 1 file changed, 43 insertions(+) diff --git a/src/Microsoft.Health.Dicom.Core.UnitTests/Features/Store/DicomStoreServiceTests.cs b/src/Microsoft.Health.Dicom.Core.UnitTests/Features/Store/DicomStoreServiceTests.cs index 63a332c8b5..8c1e26c6b9 100644 --- a/src/Microsoft.Health.Dicom.Core.UnitTests/Features/Store/DicomStoreServiceTests.cs +++ b/src/Microsoft.Health.Dicom.Core.UnitTests/Features/Store/DicomStoreServiceTests.cs @@ -27,6 +27,7 @@ using Microsoft.Health.Dicom.Core.Messages.Store; using Microsoft.Health.Dicom.Tests.Common; using NSubstitute; +using NSubstitute.ExceptionExtensions; using Xunit; using DicomValidationException = FellowOakDicom.DicomValidationException; @@ -542,6 +543,48 @@ public async Task GivenRequiredStudyInstanceUid_WhenProcessed_ThenItShouldBePass await ExecuteAndValidateAsync(dicomInstanceEntry); } + [Fact] + public async Task GivenFetchCancellation_WhenProcessed_ThenItShouldHaveThrown() + { + using CancellationTokenSource tokenSource = new CancellationTokenSource(); + IDicomInstanceEntry dicomInstanceEntry = Substitute.For(); + dicomInstanceEntry.GetDicomDatasetAsync(tokenSource.Token).Returns(ValueTask.FromException(new TaskCanceledException())); + + await Assert.ThrowsAsync(() => _storeService.ProcessAsync( + new IDicomInstanceEntry[] { dicomInstanceEntry }, + null, + cancellationToken: tokenSource.Token)); + } + + [Fact] + public async Task GivenValidationCancellation_WhenProcessed_ThenItShouldHaveThrown() + { + using CancellationTokenSource tokenSource = new CancellationTokenSource(); + IDicomInstanceEntry dicomInstanceEntry = Substitute.For(); + dicomInstanceEntry.GetDicomDatasetAsync(tokenSource.Token).Returns(_dicomDataset1); + _dicomDatasetValidator.ValidateAsync(_dicomDataset1, null, tokenSource.Token).ThrowsAsync(); + + await Assert.ThrowsAsync(() => _storeService.ProcessAsync( + new IDicomInstanceEntry[] { dicomInstanceEntry }, + null, + cancellationToken: tokenSource.Token)); + } + + [Fact] + public async Task GivenStowCancellation_WhenProcessed_ThenItShouldHaveThrown() + { + using CancellationTokenSource tokenSource = new CancellationTokenSource(); + IDicomInstanceEntry dicomInstanceEntry = Substitute.For(); + dicomInstanceEntry.GetDicomDatasetAsync(tokenSource.Token).Returns(_dicomDataset1); + _storeOrchestrator.StoreDicomInstanceEntryAsync(dicomInstanceEntry, tokenSource.Token).ThrowsAsync(new DataStoreException(new TaskCanceledException())); + + Exception actual = await Assert.ThrowsAsync(() => _storeService.ProcessAsync( + new IDicomInstanceEntry[] { dicomInstanceEntry }, + null, + cancellationToken: tokenSource.Token)); + Assert.IsType(actual.InnerException); + } + private Task ExecuteAndValidateAsync(params IDicomInstanceEntry[] dicomInstanceEntries) => ExecuteAndValidateAsync(requiredStudyInstanceUid: null, dicomInstanceEntries); From 8f6fffe320d7031ccf86c043ad17d96129a11548 Mon Sep 17 00:00:00 2001 From: Will Sugarman Date: Thu, 3 Aug 2023 10:55:58 -0700 Subject: [PATCH 3/3] Use OperationCanceledException instead --- src/Microsoft.Health.Dicom.Core/Features/Store/StoreService.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Microsoft.Health.Dicom.Core/Features/Store/StoreService.cs b/src/Microsoft.Health.Dicom.Core/Features/Store/StoreService.cs index 4f77f85d3b..4eb12a94cf 100644 --- a/src/Microsoft.Health.Dicom.Core/Features/Store/StoreService.cs +++ b/src/Microsoft.Health.Dicom.Core/Features/Store/StoreService.cs @@ -232,7 +232,7 @@ public async Task ProcessAsync( { throw; } - catch (DataStoreException dse) when (dse.InnerException is TaskCanceledException) + catch (DataStoreException dse) when (dse.InnerException is OperationCanceledException) { throw; }