Skip to content

Conversation

@NaluTripician
Copy link
Contributor

Pull Request Template

Description

There is a rare case where a NullReferenceException can occur when cloning headers in the Headers class, in request hedging scenarions

System.ArgumentNullException: Value cannot be null. (Parameter 'value')
at Microsoft.Azure.Cosmos.Headers.Clone()
at Microsoft.Azure.Cosmos.RequestMessage.Clone(ITrace newTrace, CloneableStream cloneContent)
at await Microsoft.Azure.Cosmos.CrossRegionHedgingAvailabilityStrategy.CloneAndSendAsync(?)
at await Microsoft.Azure.Cosmos.CrossRegionHedgingAvailabilityStrategy.ExecuteAvailabilityStrategyAsync(?)
at await Microsoft.Azure.Cosmos.Handlers.RequestInvokerHandler.SendAsync(?)
at await Microsoft.Azure.Cosmos.Handlers.RequestInvokerHandler.SendAsync(?)
at await Microsoft.Azure.Cosmos.ContainerCore.ProcessItemStreamAsync(?)
at await Microsoft.Azure.Cosmos.ContainerCore.ReadItemStreamAsync(?)
at await Microsoft.Azure.Cosmos.ClientContextCore.RunWithDiagnosticsHelperAsync(?)
at await Microsoft.Azure.Cosmos.ClientContextCore.OperationHelperWithRootTraceAsync(?)

This pull request introduces a null check in the Clone method of the Headers class and adds new unit tests to verify the cloning functionality, including handling of null values.

Enhancements to Headers class cloning:

New unit tests for Headers class:

Type of change

Please delete options that are not relevant.

  • [] Bug fix (non-breaking change which fixes an issue)

Closing issues

To automatically close an issue: closes #5093

@NaluTripician NaluTripician added auto-merge Enables automation to merge PRs Hedging Any issue/feature request related to request hedging labels Mar 27, 2025
@NaluTripician NaluTripician self-assigned this Mar 27, 2025
@microsoft-github-policy-service microsoft-github-policy-service bot enabled auto-merge (squash) March 27, 2025 19:11
foreach (string key in this.CosmosMessageHeaders.AllKeys())
{
clone.Add(key, this.CosmosMessageHeaders.Get(key));
string value = this.CosmosMessageHeaders.Get(key);
Copy link
Member

Choose a reason for hiding this comment

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

Is it expected that there be nulls in there? Instead of (or as well as) this should we be investigating how the null got in there?

Copy link
Member

Choose a reason for hiding this comment

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

I assumed that it's a concurrency issue, otherwise +1

Copy link
Member

Choose a reason for hiding this comment

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

What concurrency issue? Someone is writing to the headers collection on one thread while we're reading? That's not safe.

auto-merge was automatically disabled May 20, 2025 18:27

Pull request was closed

@NaluTripician
Copy link
Contributor Author

Fixed in: #5189

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

Labels

auto-merge Enables automation to merge PRs Hedging Any issue/feature request related to request hedging

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

Occasional ArgumentNullException when CrossRegionalHedging feature enabled

5 participants