Skip to content

Fix concurrent request handling for OpenAPI documents #57972

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

Merged
merged 5 commits into from
Sep 22, 2024
Merged

Fix concurrent request handling for OpenAPI documents #57972

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
89b48ee
fix: Allow concurrent requests
xC0dex Sep 19, 2024
0a8373f
test: Update test
xC0dex Sep 19, 2024
cb01ee0
test: Use Parallel.ForEachAsync
xC0dex Sep 20, 2024
b02bdc5
feat: Use valueFactory overload
xC0dex Sep 21, 2024
603b836
feat: Pass valueFactory directly
xC0dex Sep 21, 2024
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
15 changes: 5 additions & 10 deletions src/OpenApi/src/Services/Schemas/OpenApiSchemaStore.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System.Collections.Concurrent;
using System.IO.Pipelines;
using System.Text.Json.Nodes;
using Microsoft.AspNetCore.Http;
Expand All @@ -14,7 +15,7 @@ namespace Microsoft.AspNetCore.OpenApi;
/// </summary>
internal sealed class OpenApiSchemaStore
{
private readonly Dictionary<OpenApiSchemaKey, JsonNode> _schemas = new()
private readonly ConcurrentDictionary<OpenApiSchemaKey, JsonNode> _schemas = new()
{
// Pre-populate OpenAPI schemas for well-defined types in ASP.NET Core.
[new OpenApiSchemaKey(typeof(IFormFile), null)] = new JsonObject
Expand Down Expand Up @@ -48,8 +49,8 @@ internal sealed class OpenApiSchemaStore
},
};

public readonly Dictionary<OpenApiSchema, string?> SchemasByReference = new(OpenApiSchemaComparer.Instance);
private readonly Dictionary<string, int> _referenceIdCounter = new();
public readonly ConcurrentDictionary<OpenApiSchema, string?> SchemasByReference = new(OpenApiSchemaComparer.Instance);
private readonly ConcurrentDictionary<string, int> _referenceIdCounter = new();

/// <summary>
/// Resolves the JSON schema for the given type and parameter description.
Expand All @@ -59,13 +60,7 @@ internal sealed class OpenApiSchemaStore
/// <returns>A <see cref="JsonObject" /> representing the JSON schema associated with the key.</returns>
public JsonNode GetOrAdd(OpenApiSchemaKey key, Func<OpenApiSchemaKey, JsonNode> valueFactory)
{
if (_schemas.TryGetValue(key, out var schema))
{
return schema;
}
var targetSchema = valueFactory(key);
_schemas.Add(key, targetSchema);
return targetSchema;
return _schemas.GetOrAdd(key, valueFactory(key));
Copy link
Member

Choose a reason for hiding this comment

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

@BrennanConroy shared some feedback here that we can use a different overload of the GetOrAdd method here to avoid another possible race condition:

Suggested change
return _schemas.GetOrAdd(key, valueFactory(key));
return _schemas.GetOrAdd(key, valueFactory, key);

With this pattern, the GetOrAdd method will do a check for the schema key in the dictionary before calling into the factory.

The current overload used will always call the valueFactory even if the key already exists in the dictionary.

In this case, the valueFactory were calling is an invocation into the JsonSchemaMapper. I dunno if it is particularly prone to race issues but it still doesn't hurt to avoid unnecessarily calling into it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the feedback. One small PR and I already learned a lot!
Unfortunately, the suggested overload does not work because it expects a valueFactory of type Func<TKey, TArg, TValue>. Luckily, there is another overload, so I could do the following:

- return _schemas.GetOrAdd(key, valueFactory(key));
+ return _schemas.GetOrAdd(key, _ => valueFactory(key));

Copy link
Member

Choose a reason for hiding this comment

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

I think this should work, and will avoid needing to allocate a closure:

return _schemas.GetOrAdd(key, valueFactory);

}

/// <summary>
Expand Down
3 changes: 2 additions & 1 deletion src/OpenApi/src/Transformers/Implementations/OpenApiSchemaReferenceTransformer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System.Collections.Concurrent;
using System.Linq;
using Microsoft.Extensions.DependencyInjection;
using Microsoft.OpenApi.Models;
Expand Down Expand Up @@ -85,7 +86,7 @@ public Task TransformAsync(OpenApiDocument document, OpenApiDocumentTransformerC
/// <param name="schema">The inline schema to replace with a reference.</param>
/// <param name="schemasByReference">A cache of schemas and their associated reference IDs.</param>
/// <param name="isTopLevel">When <see langword="true" />, will skip resolving references for the top-most schema provided.</param>
internal static OpenApiSchema? ResolveReferenceForSchema(OpenApiSchema? schema, Dictionary<OpenApiSchema, string?> schemasByReference, bool isTopLevel = false)
internal static OpenApiSchema? ResolveReferenceForSchema(OpenApiSchema? schema, ConcurrentDictionary<OpenApiSchema, string?> schemasByReference, bool isTopLevel = false)
{
if (schema is null)
{
Expand Down
32 changes: 32 additions & 0 deletions ...t/Microsoft.AspNetCore.OpenApi.Tests/Integration/OpenApiDocumentConcurrentRequestTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System.Net;
using System.Net.Http;

namespace Microsoft.AspNetCore.OpenApi.Tests.Integration;

public class OpenApiDocumentConcurrentRequestTests(SampleAppFixture fixture) : IClassFixture<SampleAppFixture>
{
[Fact]
public async Task MapOpenApi_HandlesConcurrentRequests()
{
// Arrange
var client = fixture.CreateClient();
var requests = new List<Task<HttpResponseMessage>>
{
client.GetAsync("/openapi/v1.json"),
client.GetAsync("/openapi/v1.json"),
client.GetAsync("/openapi/v1.json")
};

// Act
var results = await Task.WhenAll(requests);
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if using Parallel with a bigger number of operations would make this more likely to always run requests that race against each other?

Copy link
Contributor Author

@xC0dex xC0dex Sep 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks. That was an excellent idea. I switched to Parallel.ForAsync and it turned out that the _operationTransformerContextCache field must also be a ConcurrentDictionary.


// Assert
foreach (var result in results)
{
Assert.Equal(HttpStatusCode.OK, result.StatusCode);
}
}
}
Loading