Skip to content

Fix race creating options for documents requested concurrently - #1229

Open
OptoCloud wants to merge 4 commits into
dotnet:mainfrom
OptoCloud:main
Open

OptoCloud wants to merge 4 commits into
dotnet:mainfrom
OptoCloud:main

Conversation

@OptoCloud

Copy link
Copy Markdown
  • 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

VersionedOpenApiOptionsFactory is registered as a singleton but kept the context of the options being created in an instance field.
CreateAndConfigure sets the field, calls Create, and clears it again.
When the options of two documents are created at the same time, one thread can clear the field between another thread's null check and its use.

This produces one of two results, and OptionsCache stores either for the lifetime of the process:

  • a NullReferenceException in VersionedOpenApiOptionsFactory.Create, so the document endpoint returns a 500
  • Create sees a null or mismatched context and returns DefaultOptions(), so the document is served without any of the versioned configuration

The context is now stored in a [ThreadStatic] field. Create is only ever called synchronously from CreateAndConfigure on the same thread, so each thread sees only its own context.
The reset also moved into a finally block so an exception during configuration no longer leaves a stale context behind.

A regression test starts a new host 50 times, requests /openapi/v1.json and /openapi/v2.json at the same moment, and asserts that each document has its own API version and paths.
It fails against the current code with the NullReferenceException and passes with this change.

Fixes #1228

@OptoCloud

Copy link
Copy Markdown
Author

@dotnet-policy-service agree

@OptoCloud

Copy link
Copy Markdown
Author

@commonsensesoftware just following up on this PR.
The race condition in my tests causes OpenAPI document generation to consistently fail with a 500 or just returning empty documents.

Is there anything you'd like me to change or clarify before this can be merged?
If not, do you have a rough timeline for when it might be reviewed and merged?

This is currently breaking API documentation generation unless significant workarounds are done, so getting a fix in would be really helpful.

@commonsensesoftware

Copy link
Copy Markdown
Collaborator

@OptoCloud thanks for taking the time put this together. It looks like a simple, straight forward approach. I intend to review and think about this a bit more to make sure this is the right direction to go. This might be a good stop gap or might be the real final answer.

The internals are super wonky, but I wasn't able to get MS to unseal the options. That would make so much of this a lot less clunky. In retrospect, it works, but I'm not really pleased with the design.

I've been backed up a bit lately, but I'll put eyes on this ASAP.

This branch has not been deployed

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

VersionedOpenApiOptionsFactory race: concurrent document requests cause NullReferenceException or unconfigured document

2 participants