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.

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

Jump to bottom

Fix: Concurrent request for OpenApi documents #57972

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Fix: Concurrent request for OpenApi documents #57972

wants to merge 2 commits into from
+39 −11

Conversation

xC0dex
Copy link

@xC0dex xC0dex commented Sep 19, 2024

Fix: Concurrent request for OpenApi documents

  • You've read the Contributor Guide and Code of Conduct.
  • You've included unit or integration tests for your change, where applicable.
  • You've included inline docs for your change, where applicable.
  • There's an open issue for the PR that you are making. If you'd like to propose a new feature or change, please open an issue to discuss the change or find an existing issue.

Description

This PR allows concurrent requests for OpenApi documents by switching from a Dictionary to a ConcurrentDictionary. The fields _schemas and SchemasByReference had to be changed to ConcurrentDictionary to pass the test. Additionally, I updated _referenceIdCounter for consistency.

Fixes #57876

@xC0dex xC0dex requested review from captainsafia and a team as code owners September 19, 2024 19:39
@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label Used by the dotnet-issue-labeler to label those issues which couldn't be triaged automatically label Sep 19, 2024
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Sep 19, 2024
@dotnet-policy-service Dotnet Policy Service
Copy link
Contributor

Thanks for your PR, @xC0dex. Someone from the team will get assigned to your PR shortly and we'll get it reviewed.

@xC0dex
Copy link
Author

xC0dex commented Sep 19, 2024

@dotnet-policy-service agree

martincostello
martincostello reviewed Sep 19, 2024
View reviewed changes
...test/Microsoft.AspNetCore.OpenApi.Tests/Integration/OpenApiDocumentConcurrentRequestTests.cs
};

// 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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@martincostello martincostello martincostello left review comments

@captainsafia captainsafia Awaiting requested review from captainsafia captainsafia is a code owner

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
community-contribution Indicates that the PR has been added by a community member needs-area-label Used by the dotnet-issue-labeler to label those issues which couldn't be triaged automatically
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Minimal API Group with OpenAPI and Return Type of Result<TResult> creates incorrect OpenAPI document.
2 participants
@xC0dex @martincostello