NullReferenceException in QueryStringApiVersionReader.cs

[phalpin]

aspnet-api-versioning/src/AspNetCore/WebApi/src/Asp.Versioning.Http/QueryStringApiVersionReader.cs

Line 38 in d71f42d

var values = request.Query[names[i]];

Getting an intermittent null reference exception in one of our applications -

Swapping from QueryStringApiVersionReader to HeaderApiVersionReader alleviates the issue. Noticed in the codebase (we're currently using 5.0 - so a little behind) - in 5.0 (and in the latest, I think?) the reference to the request's querystring is not calling TryGetValue - from our stack traces it looks like that's where the null is originating.

We're ok for the moment swapping to just header version parsing - however, wanted to bubble up this issue, see if anyone else had the same problem.

Thanks!

[commonsensesoftware]

The issue here isn't the value returned by the indexed Query property. The values returned are StringValues, which is a struct. It can never be null.

https://github.com/dotnet/aspnetcore/blob/main/src/Http/Http.Features/src/IQueryCollection.cs#L85

Based on the stack trace, the issue appears to that the HttpRequest.Query property itself returns a null IQueryCollection. I did some spelunking in:

https://github.com/dotnet/aspnetcore/blob/main/src/Http/Http/src/Internal/DefaultHttpRequest.cs

and

https://github.com/dotnet/aspnetcore/blob/main/src/Extensions/Features/src/FeatureReferences.cs

It seems this should never happen. The code was all updated with nullable annotations and this isn't a property that is supposed to allow null. I see commit dates back to Nov 2021, which would coincide with the .NET 5 release. The only other thing I can think of is that something is explicitly setting this property to null (in your code?). That would be strange too, but I can't explain this. This is a pretty hot path that has been exercised this way for several major versions, but it's never been reported before. I can't rule out a bug, but it seems something else may be afoot.

If this is a bug, there isn't really a way for me to get it out to you. I can make the fix, but there's no way for me to publish it. Microsoft has continued to drag its feet on enabling that for me and, at this point, I've pretty much surrendered to the fact it is unlikely to ever happen. The only other option is to publish a standalone release that you'd have consume by ingesting it into your package management system like I did with 5.1.

I'm not sure what the options for you to upgrade are. You didn't mention what version of .NET you're targeting. If it's >= .NET 6.0, that is not officially supported by the 5.x release. There is a strong pairing between .NET major releases. It might work, but there have been cases where things didn't always work as expected.

It's been about 2 years since I've touched any of the .NET 5.0 stuff, but I'm happy to help you investigate and come up with a workable solution.

[phalpin]

Hey there,

Yeah we're targeting 6.0 - we'll try playing around with moving the version up (I think there were namespace changes in there?) in the next few sprints. I agree it's very, very bizarre. The error itself is extremely intermittent - prior to this release we just did, we were getting this about 1/10000 requests, now since our latest release (no idea honestly looking at the diffs) it's like 1/1000. For now, we've just disabled the querystring versioning since it was only a shim anyway for testing things for our clients, but we'll try upgrading initially, see if it fixes the problem. If not, then we'll go ahead and inherit from the query string api version reader, override that value and drop a /shrug catch in there for this weird case.

We're definitely not setting that value to null anywhere, and there's really no rhyme or reason that I can pick out as to why we're getting nulls there occasionally (even on the EXACT same request - IE, issuing the exact same request 1000 times now will yield a 500 sporadically). No fancy middleware or anything screwing with the request, so at this point I'm at a loss.

[commonsensesoftware]

Agreed.

I got so hung up on trying to fix it for you 😉 ... that - yeah - you can totally extend or fork the implementation into a reader that tests and guards against null. No update needed. When/if you do decide to update, there will be a number of mostly trivial, but annoying and tedious updates to changes packages, namespaces, etc. I really tried to avoid that, but it was that or do nothing. All of the example projects are updated for reference so that should be useful in the migration process.

This only other place I can think of where this behavior might be happening is you have some kind of work at the end of the pipeline. If you tried to get the requested API version at the end of the pipeline, it's possible the IApiVersioningFeature has been unset and will be recreated. This could lead to a path where the features are uninitialized and might produce null. Some type of logging late in the pipeline perhaps 🤔. It's a wild guess, but perhaps something else to consider. This would be especially true if things had been working for a long time and now you are suddenly facing this issue. Something has changed.

[commonsensesoftware]

Did you ever discover more information or were you able to resolve the issue?

[phalpin]

Yeah this has led us down quite the rabbit hole - We think it's actually related to this:

dotnet/SqlClient#1634

We think that somehow the thread safety issue of httpcontextaccessor is getting tripped because there's parallel dependency telemetry data that's getting sent up to application insights. We noted that if we killed app insights altogether, the problem went away. Still just a shot in the dark, but we've upgraded everything from system.data.sqlclient to microsoft.data.sqlclient in hopes that we could resolve this issue. We'll see what happens when this next release goes out. Crossing fingers.

[phalpin]

++ Sorry, extra piece of context - we have a telemetry initializer that relies on httpcontextaccessor that grabs some info to append into logs (correlation id and all that)

[commonsensesoftware]

Thanks for the clarification. I don't I would have ever guessed something like that. 😸

[phalpin]

Yeah. You can consider this issue closed if you'd like in the interim, I'll update once we have some actually real runtime data to look at once this gets deployed to prod.

[commonsensesoftware]

Since this issue appears to be external, I'm going to close out per @phalpin. If there is something additional to add for future readers, feel free to do so. The issue can be reopened later if necessary.