Skip to content

Commit

Permalink
[Bugfix] HeaderDictionary - Support concurrent operations (#3729)
Browse files Browse the repository at this point in the history 
* Bundle - Concurrent access

* Introducing new ThreadSafeHeaderDictionary

* Adding tests

* Fix conccurent access to http headers.

* Fix concurrent access to http headers.

* New test to validate IHeaderDictionary clones.

* Using statements. Small refactorings

* remove clone

* Expose FHIR operation outcome in case of errors

* Adding more details to the exception
  • Loading branch information
@fhibf
fhibf authored Mar 20, 2024
1 parent fbfc674 commit 6afd273
Show file tree
Hide file tree
Showing 8 changed files with 50 additions and 27 deletions.
20 changes: 13 additions & 7 deletions src/Microsoft.Health.Fhir.Api/Features/ActionResults/ResourceActionResult.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
// Licensed under the MIT License (MIT). See LICENSE in the repo root for license information.
// -------------------------------------------------------------------------------------------------

using System;
using System.Collections.Generic;
using System.Net;
using System.Threading.Tasks;
Expand All @@ -21,9 +22,11 @@ public abstract class ResourceActionResult<TResult> : ActionResult, IResourceAct
{
protected ResourceActionResult()
{
Headers = new HeaderDictionary();
}

protected ResourceActionResult(TResult result)
: this()
{
Result = result;
}
Expand All @@ -47,9 +50,9 @@ protected ResourceActionResult(TResult result, HttpStatusCode statusCode)
/// <summary>
/// Gets or sets the action result Headers.
/// </summary>
internal IHeaderDictionary Headers { get; } = new HeaderDictionary();
internal IHeaderDictionary Headers { get; }

public override Task ExecuteResultAsync(ActionContext context)
public override async Task ExecuteResultAsync(ActionContext context)
{
EnsureArg.IsNotNull(context, nameof(context));

Expand All @@ -68,12 +71,15 @@ public override Task ExecuteResultAsync(ActionContext context)

foreach (KeyValuePair<string, StringValues> header in Headers)
{
if (response.Headers.ContainsKey(header.Key))
try
{
response.Headers.Remove(header.Key);
response.Headers[header.Key] = header.Value;
}
catch (InvalidOperationException ioe)
{
// Catching operations that change non-concurrent collections.
throw new InvalidOperationException($"Failed to set header '{header.Key}'.", ioe);
}

response.Headers.Add(header);
}

ActionResult result;
Expand All @@ -86,7 +92,7 @@ public override Task ExecuteResultAsync(ActionContext context)
result = new ObjectResult(GetResultToSerialize());
}

return result.ExecuteResultAsync(context);
await result.ExecuteResultAsync(context);
}

protected virtual object GetResultToSerialize()
Expand Down
28 changes: 19 additions & 9 deletions src/Microsoft.Health.Fhir.Core/Features/Conformance/SystemConformanceProvider.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@
using System;
using System.Collections.Generic;
using System.Diagnostics;
using System.Linq;
using System.Text;
using System.Threading;
using System.Threading.Tasks;
using EnsureThat;
Expand Down Expand Up @@ -88,15 +90,7 @@ public SystemConformanceProvider(
_contextAccessor = contextAccessor;
_searchParameterStatusManager = searchParameterStatusManager;

if (!string.IsNullOrEmpty(_configuration.Value.Versioning.Default))
{
_logger.LogInformation("Default version is:{VersioningDefault}.", _configuration.Value.Versioning.Default);

foreach (var resourcetype in _configuration.Value.Versioning.ResourceTypeOverrides)
{
_logger.LogInformation("{ResourceTypeKey} version overridden to:{ResourceTypeVersioningOverride}.", resourcetype.Key, resourcetype.Value);
}
}
LogVersioningPolicyConfiguration();
}

public override async Task<ResourceElement> GetCapabilityStatementOnStartup(CancellationToken cancellationToken = default(CancellationToken))
Expand Down Expand Up @@ -361,5 +355,21 @@ public override async Task<ResourceElement> GetMetadata(CancellationToken cancel
_metadataSemaphore?.Release();
}
}

private void LogVersioningPolicyConfiguration()
{
if (!string.IsNullOrEmpty(_configuration.Value.Versioning.Default) || _configuration.Value.Versioning.ResourceTypeOverrides.Any())
{
StringBuilder versioning = new StringBuilder();
versioning.AppendLine($"Default version is: '{_configuration.Value.Versioning.Default ?? "(default)"}'.");

foreach (var resourceTypeVersioning in _configuration.Value.Versioning.ResourceTypeOverrides)
{
versioning.AppendLine($"'{resourceTypeVersioning.Key}' version overridden to: '{resourceTypeVersioning.Value}'.");
}

_logger.LogInformation(versioning.ToString());
}
}
}
}
4 changes: 2 additions & 2 deletions src/Microsoft.Health.Fhir.Shared.Api.UnitTests/Features/ActionResults/FhirResultTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ public void GivenANotFoundStatus_WhenReturningAResult_ThenTheStatusCodeIsSetCorr
}

[Fact]
public void GivenAFhirResult_WhenHeadersThatAlreadyExistsInResponseArePassed_ThenDuplicteHeadersAreRemoved()
public async Task GivenAFhirResult_WhenHeadersThatAlreadyExistsInResponseArePassed_ThenDuplicteHeadersAreRemoved()
{
var result = FhirResult.Gone();
var context = new ActionContext
Expand All @@ -94,7 +94,7 @@ public void GivenAFhirResult_WhenHeadersThatAlreadyExistsInResponseArePassed_The
result.Headers["testKey2"] = "2";
context.HttpContext.Response.Headers["testKey2"] = "1";

result.ExecuteResultAsync(context);
await result.ExecuteResultAsync(context);

Assert.Null(result.Result);
Assert.Equal(HttpStatusCode.Gone, result.StatusCode.GetValueOrDefault());
Expand Down
4 changes: 2 additions & 2 deletions src/Microsoft.Health.Fhir.Shared.Api/Features/ActionResults/OperationVersionsResult.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,11 +40,11 @@ public OperationVersionsResult(VersionsResult versionsResult, HttpStatusCode sta
_versionsResult = versionsResult;
}

public override Task ExecuteResultAsync(ActionContext context)
public override async Task ExecuteResultAsync(ActionContext context)
{
_httpContext = context.HttpContext;
_acceptHeaders = _httpContext.Request.GetTypedHeaders().Accept;
return base.ExecuteResultAsync(context);
await base.ExecuteResultAsync(context);
}

protected override object GetResultToSerialize()
Expand Down
11 changes: 6 additions & 5 deletions src/Microsoft.Health.Fhir.Shared.Api/Features/Resources/Bundle/BundleHandler.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
// -------------------------------------------------------------------------------------------------

using System;
using System.Collections.Concurrent;
using System.Collections.Generic;
using System.Diagnostics;
using System.IO;
Expand Down Expand Up @@ -80,7 +81,7 @@ public partial class BundleHandler : IRequestHandler<BundleRequest, BundleRespon
private readonly ILogger<BundleHandler> _logger;
private readonly HTTPVerb[] _verbExecutionOrder;
private readonly List<int> _emptyRequestsOrder;
private readonly Dictionary<string, (string resourceId, string resourceType)> _referenceIdDictionary;
private readonly ConcurrentDictionary<string, (string resourceId, string resourceType)> _referenceIdDictionary;
private readonly TransactionBundleValidator _transactionBundleValidator;
private readonly ResourceReferenceResolver _referenceResolver;
private readonly IAuditEventTypeMapping _auditEventTypeMapping;
Expand Down Expand Up @@ -151,7 +152,7 @@ public BundleHandler(
_requestServices = outerHttpContext.RequestServices;
_originalRequestBase = outerHttpContext.Request.PathBase;
_emptyRequestsOrder = new List<int>();
_referenceIdDictionary = new Dictionary<string, (string resourceId, string resourceType)>();
_referenceIdDictionary = new ConcurrentDictionary<string, (string resourceId, string resourceType)>();

// Retrieve bundle processing logic.
_bundleProcessingLogic = GetBundleProcessingLogic(outerHttpContext, _logger);
Expand Down Expand Up @@ -780,8 +781,8 @@ private static void SetupContexts(
httpContext.Request.GetDisplayUrl(),
requestContext.BaseUri.OriginalString,
requestContext.CorrelationId,
httpContext.Request.Headers,
httpContext.Response.Headers)
requestHeaders: httpContext.Request.Headers,
responseHeaders: httpContext.Response.Headers)
{
Principal = requestContext.Principal,
ResourceType = resourceType?.ToString(),
Expand Down Expand Up @@ -839,7 +840,7 @@ private static void SetupContexts(
}
}

private void PopulateReferenceIdDictionary(IEnumerable<EntryComponent> bundleEntries, Dictionary<string, (string resourceId, string resourceType)> idDictionary)
private void PopulateReferenceIdDictionary(IEnumerable<EntryComponent> bundleEntries, IDictionary<string, (string resourceId, string resourceType)> idDictionary)
{
foreach (EntryComponent entry in bundleEntries)
{
Expand Down
6 changes: 6 additions & 0 deletions src/Microsoft.Health.Fhir.Shared.Client/FhirClientException.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
using System.Net.Http.Headers;
using System.Text;
using Hl7.Fhir.Model;
using Hl7.Fhir.Serialization;

namespace Microsoft.Health.Fhir.Client
{
Expand Down Expand Up @@ -102,6 +103,11 @@ private string FormatMessage()
message.Append("Timestamp: ").AppendLine(DateTime.UtcNow.ToString("o"));
message.Append("Health Check Result: ").Append(HealthCheckResult.ToString()).Append('(').Append((int)HealthCheckResult).AppendLine(")");

if (Response.Resource != null)
{
message.Append("OperationOutcome: ").AppendLine(Response.Resource.ToJson());
}

if (appendResponseInfo)
{
message.Append("Response Info: ").AppendLine(responseInfo);
Expand Down
2 changes: 1 addition & 1 deletion src/Microsoft.Health.Fhir.Shared.Core/Features/Resources/ResourceReferenceResolver.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ public ResourceReferenceResolver(ISearchService searchService, IQueryStringParse
_queryStringParser = queryStringParser;
}

public async Task ResolveReferencesAsync(Resource resource, Dictionary<string, (string resourceId, string resourceType)> referenceIdDictionary, string requestUrl, CancellationToken cancellationToken)
public async Task ResolveReferencesAsync(Resource resource, IDictionary<string, (string resourceId, string resourceType)> referenceIdDictionary, string requestUrl, CancellationToken cancellationToken)
{
IEnumerable<ResourceReference> references = resource.GetAllChildren<ResourceReference>();

Expand Down
2 changes: 1 addition & 1 deletion test/Microsoft.Health.Fhir.Shared.Tests.E2E/Rest/BundleBatchTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ public async Task GivenAValidBundle_WhenSubmittingABatch_ThenSuccessIsReturnedFo

if (processingLogic == FhirBundleProcessingLogic.Parallel)
{
// Duplicated records. Only one should successed. As the requests are processed in parallel,
// Duplicated records. Only one should succeed. As the requests are processed in parallel,
// it's not possible to pick the one that will be processed.
if (resource.Entry[2].Response.Status == "200")
{
Expand Down

0 comments on commit 6afd273

Please sign in to comment.