Conversation
|
|
||
| var queryUri = queryUrl.Uri; | ||
| var results = await _client.GetJObjectAsync( | ||
| AutoCompleteModel results = await _client.ProcessStreamAsync( |
There was a problem hiding this comment.
I am surprised that AutoCompleteModel is always non-null. Is that correct?
Does this library not have nullable enabled?
There was a problem hiding this comment.
Not yet. It's #nullable disabled for now, but it's a work in progress
| var results = await _client.GetJObjectAsync( | ||
| AutoCompleteModel results = await _client.ProcessStreamAsync( | ||
| new HttpSourceRequest(queryUri, logger), | ||
| async stream => |
There was a problem hiding this comment.
I don't know what the team code style is, if they link lamdas defined ahead of the method call or within it.
Nigusu-Allehu
force-pushed
the
dev-nyenework-autocomplete-stj
branch
from
01dc4d3 to
6f4e797
Compare
Nigusu-Allehu
requested review from
donnie-msft and
jebriede
and removed request for
a team
Nigusu-Allehu
changed the base branch from
dev-feature-nsj-stj-migration
to
dev
|
|
||
| var queryUri = queryUrl.Uri; | ||
|
|
||
| if (NuGetFeatureFlags.UseNSJDeserializationFeatureSwitch || NuGetFeatureFlags.IsNSJDeserializationEnabledByEnvironment(_environmentVariableReader)) |
There was a problem hiding this comment.
We should learn from #7266, because this pattern here will read the environment variable every time, and on .NET Framework that causes memory allocations every time. Making it both testable and avoid allocations is difficult, but we need to find a solution.
When that other PR was open, I remember someone sharing an idea that EnvironmentVariableWrapper could cache values in a dictionary, so it avoids calling the BCL API that causes allocations after the first time. I don't love the idea because adding to static collections feels like a memory leak, but at the least the number of environment variables we check is low. If we can't find a better solution, then implementing this will be an easy fix.
There was a problem hiding this comment.
This should have been caching. The idea was to pass null in production and the caching is done in the NuGetFeatureClassFlag class. And pass env value during test to avoid the caching.
There was a problem hiding this comment.
Line 37 isn't using null, it's using EnvironmentVariableWrapper.Instance, so it's not going to cache. This shows a risk of this design. It's very prone to error, preventing caching. Even if you manually change it, I'm wondering if AI generated code is going to try to optimisticly keep using EnvironmentVariableWrapper.Instance since that's what's done in most other parts of the code.
From an API design point of view, the best APIs are hard or impossible to use wrong. So, fixing the null value is an option, but if we can find another design, we might be able to reduce risk of repeating the same error.
There was a problem hiding this comment.
To reduce the risk of future mistakes, I've added an XML docomment on IsLegacyJsonDeserializationEnabledByEnvironment that explicitly says to pass null in production for caching, and only pass a reader in tests. Since it's internal, we can revisit the design at any point during the migration if we find something cleaner.
3 participants
Bug
Fixes:
Related: NuGet/Home#14846
Description
Makes use of features introduced in #7313.
Replaces NSJ deserialization with (stj)JsonSerializer.DeserializeAsync into a new AutoCompleteModel POCO. STJ is now the default path. Users can opt back into NSJ by setting NUGET_USE_NSJ_DESERIALIZATION=true or via the NuGet.UseNSJDeserialization AppContext switch.
PR Checklist