Skip to content

Commit 555651e

Browse files
zivkanzivkan
authored
Reduce memory allocations in HttpFileSystembasedFindPackageByIdResource (#6300)
1 parent 7e1559a commit 555651e

File tree

6 files changed

+214
-196
lines changed

6 files changed

+214
-196
lines changed

‎src/NuGet.Core/NuGet.Protocol/GlobalSuppressions.cs

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -63,9 +63,6 @@
6363
[assembly: SuppressMessage("Build", "CA1303:Method 'Task<DownloadResourceResult> GetDownloadResultUtility.GetDownloadResultAsync(HttpSource client, PackageIdentity identity, Uri uri, PackageDownloadContext downloadContext, string globalPackagesFolder, ILogger logger, CancellationToken token)' passes a literal string as parameter 'message' of a call to 'InvalidOperationException.InvalidOperationException(string message)'. Retrieve the following string(s) from a resource table instead: \"Reached an unexpected point in the code\".", Justification = "<Pending>", Scope = "member", Target = "~M:NuGet.Protocol.GetDownloadResultUtility.GetDownloadResultAsync(NuGet.Protocol.HttpSource,NuGet.Packaging.Core.PackageIdentity,System.Uri,NuGet.Protocol.Core.Types.PackageDownloadContext,System.String,NuGet.Common.ILogger,System.Threading.CancellationToken)~System.Threading.Tasks.Task{NuGet.Protocol.Core.Types.DownloadResourceResult}")]
6464
[assembly: SuppressMessage("Build", "CA1062:In externally visible method 'Task HttpCacheUtility.CreateCacheFileAsync(HttpCacheResult result, HttpResponseMessage response, Action<Stream> ensureValidContents, CancellationToken cancellationToken)', validate parameter 'result' is non-null before using it. If appropriate, throw an ArgumentNullException when the argument is null or add a Code Contract precondition asserting non-null argument.", Justification = "<Pending>", Scope = "member", Target = "~M:NuGet.Protocol.HttpCacheUtility.CreateCacheFileAsync(NuGet.Protocol.HttpCacheResult,System.Net.Http.HttpResponseMessage,System.Action{System.IO.Stream},System.Threading.CancellationToken)~System.Threading.Tasks.Task")]
6565
[assembly: SuppressMessage("Build", "CA1062:In externally visible method 'HttpCacheResult HttpCacheUtility.InitializeHttpCacheResult(string httpCacheDirectory, Uri sourceUri, string cacheKey, HttpSourceCacheContext context)', validate parameter 'context' is non-null before using it. If appropriate, throw an ArgumentNullException when the argument is null or add a Code Contract precondition asserting non-null argument.", Justification = "<Pending>", Scope = "member", Target = "~M:NuGet.Protocol.HttpCacheUtility.InitializeHttpCacheResult(System.String,System.Uri,System.String,NuGet.Protocol.Core.Types.HttpSourceCacheContext)~NuGet.Protocol.HttpCacheResult")]
66-
[assembly: SuppressMessage("Build", "CA1308:In method 'BuildModel', replace the call to 'ToLowerInvariant' with 'ToUpperInvariant'.", Justification = "<Pending>", Scope = "member", Target = "~M:NuGet.Protocol.HttpFileSystemBasedFindPackageByIdResource.BuildModel(System.String,System.String,System.String)~NuGet.Protocol.HttpFileSystemBasedFindPackageByIdResource.PackageInfo")]
67-
[assembly: SuppressMessage("Build", "CA1822:Member BuildModel does not access instance data and can be marked as static (Shared in VisualBasic)", Justification = "<Pending>", Scope = "member", Target = "~M:NuGet.Protocol.HttpFileSystemBasedFindPackageByIdResource.BuildModel(System.String,System.String,System.String)~NuGet.Protocol.HttpFileSystemBasedFindPackageByIdResource.PackageInfo")]
68-
[assembly: SuppressMessage("Build", "CA1308:In method 'FindPackagesByIdAsync', replace the call to 'ToLowerInvariant' with 'ToUpperInvariant'.", Justification = "<Pending>", Scope = "member", Target = "~M:NuGet.Protocol.HttpFileSystemBasedFindPackageByIdResource.FindPackagesByIdAsync(System.String,NuGet.Protocol.Core.Types.SourceCacheContext,NuGet.Common.ILogger,System.Threading.CancellationToken)~System.Threading.Tasks.Task{System.Collections.Generic.SortedDictionary{NuGet.Versioning.NuGetVersion,NuGet.Protocol.HttpFileSystemBasedFindPackageByIdResource.PackageInfo}}")]
6966
[assembly: SuppressMessage("Build", "CA1062:In externally visible method 'Task<Tuple<bool, INuGetResource>> HttpFileSystemBasedFindPackageByIdResourceProvider.TryCreate(SourceRepository sourceRepository, CancellationToken token)', validate parameter 'sourceRepository' is non-null before using it. If appropriate, throw an ArgumentNullException when the argument is null or add a Code Contract precondition asserting non-null argument.", Justification = "<Pending>", Scope = "member", Target = "~M:NuGet.Protocol.HttpFileSystemBasedFindPackageByIdResourceProvider.TryCreate(NuGet.Protocol.Core.Types.SourceRepository,System.Threading.CancellationToken)~System.Threading.Tasks.Task{System.Tuple{System.Boolean,NuGet.Protocol.Core.Types.INuGetResource}}")]
7067
[assembly: SuppressMessage("Build", "CA1062:In externally visible method 'Task<Tuple<bool, INuGetResource>> HttpHandlerResourceV3Provider.TryCreate(SourceRepository source, CancellationToken token)', validate parameter 'source' is non-null before using it. If appropriate, throw an ArgumentNullException when the argument is null or add a Code Contract precondition asserting non-null argument.", Justification = "<Pending>", Scope = "member", Target = "~M:NuGet.Protocol.HttpHandlerResourceV3Provider.TryCreate(NuGet.Protocol.Core.Types.SourceRepository,System.Threading.CancellationToken)~System.Threading.Tasks.Task{System.Tuple{System.Boolean,NuGet.Protocol.Core.Types.INuGetResource}}")]
7168
[assembly: SuppressMessage("Build", "CA1031:Modify 'GetAsync' to catch a more specific allowed exception type, or rethrow the exception.", Justification = "<Pending>", Scope = "member", Target = "~M:NuGet.Protocol.HttpSource.GetAsync``1(NuGet.Protocol.HttpSourceCachedRequest,System.Func{NuGet.Protocol.HttpSourceResult,System.Threading.Tasks.Task{``0}},NuGet.Common.ILogger,System.Threading.CancellationToken)~System.Threading.Tasks.Task{``0}")]

‎src/NuGet.Core/NuGet.Protocol/HttpSource/HttpSource.cs

Lines changed: 94 additions & 85 deletions
Original file line numberDiff line numberDiff line change
@@ -81,116 +81,125 @@ public virtual async Task<T> GetAsync<T>(
8181
action: async lockedToken =>
8282
{
8383
cacheResult.Stream = TryReadCacheFile(request.Uri, cacheResult.MaxAge, cacheResult.CacheFile);
84-
85-
if (cacheResult.Stream != null)
84+
try
8685
{
87-
log.LogInformation(string.Format(CultureInfo.InvariantCulture, " " + Strings.Http_RequestLog, "CACHE", request.Uri));
88-
89-
// Validate the content fetched from the cache.
90-
try
86+
if (cacheResult.Stream != null)
9187
{
92-
request.EnsureValidContents?.Invoke(cacheResult.Stream);
93-
94-
cacheResult.Stream.Seek(0, SeekOrigin.Begin);
88+
log.LogInformation(string.Format(CultureInfo.InvariantCulture, " " + Strings.Http_RequestLog, "CACHE", request.Uri));
9589

96-
var httpSourceResult = new HttpSourceResult(
97-
HttpSourceResultStatus.OpenedFromDisk,
98-
cacheResult.CacheFile,
99-
cacheResult.Stream);
90+
// Validate the content fetched from the cache.
91+
try
92+
{
93+
request.EnsureValidContents?.Invoke(cacheResult.Stream);
10094

101-
return await processAsync(httpSourceResult);
102-
}
103-
catch (Exception e)
104-
{
105-
cacheResult.Stream.Dispose();
106-
cacheResult.Stream = null;
95+
cacheResult.Stream.Seek(0, SeekOrigin.Begin);
10796

108-
string message = string.Format(CultureInfo.CurrentCulture, Strings.Log_InvalidCacheEntry, request.Uri)
109-
+ Environment.NewLine
110-
+ ExceptionUtilities.DisplayMessage(e);
111-
log.LogWarning(message);
112-
}
113-
}
97+
var httpSourceResult = new HttpSourceResult(
98+
HttpSourceResultStatus.OpenedFromDisk,
99+
cacheResult.CacheFile,
100+
cacheResult.Stream);
114101

115-
Func<HttpRequestMessage> requestFactory = () =>
116-
{
117-
var requestMessage = HttpRequestMessageFactory.Create(HttpMethod.Get, request.Uri, log);
102+
return await processAsync(httpSourceResult);
103+
}
104+
catch (Exception e)
105+
{
106+
cacheResult.Stream.Dispose();
107+
cacheResult.Stream = null;
118108

119-
foreach (var acceptHeaderValue in request.AcceptHeaderValues)
120-
{
121-
requestMessage.Headers.Accept.Add(acceptHeaderValue);
109+
string message = string.Format(CultureInfo.CurrentCulture, Strings.Log_InvalidCacheEntry, request.Uri)
110+
+ Environment.NewLine
111+
+ ExceptionUtilities.DisplayMessage(e);
112+
log.LogWarning(message);
113+
}
122114
}
123115

124-
return requestMessage;
125-
};
126-
127-
Func<Task<ThrottledResponse>> throttledResponseFactory = () => GetThrottledResponse(
128-
requestFactory,
129-
request.RequestTimeout,
130-
request.DownloadTimeout,
131-
request.MaxTries,
132-
request.IsRetry,
133-
request.IsLastAttempt,
134-
request.CacheContext.SourceCacheContext.SessionId,
135-
log,
136-
lockedToken);
137-
138-
using (var throttledResponse = await throttledResponseFactory())
139-
{
140-
if (request.IgnoreNotFounds && throttledResponse.Response.StatusCode == HttpStatusCode.NotFound)
116+
Func<HttpRequestMessage> requestFactory = () =>
141117
{
142-
var httpSourceResult = new HttpSourceResult(HttpSourceResultStatus.NotFound);
118+
var requestMessage = HttpRequestMessageFactory.Create(HttpMethod.Get, request.Uri, log);
143119

144-
return await processAsync(httpSourceResult);
145-
}
120+
foreach (var acceptHeaderValue in request.AcceptHeaderValues)
121+
{
122+
requestMessage.Headers.Accept.Add(acceptHeaderValue);
123+
}
146124

147-
if (throttledResponse.Response.StatusCode == HttpStatusCode.NoContent)
125+
return requestMessage;
126+
};
127+
128+
Func<Task<ThrottledResponse>> throttledResponseFactory = () => GetThrottledResponse(
129+
requestFactory,
130+
request.RequestTimeout,
131+
request.DownloadTimeout,
132+
request.MaxTries,
133+
request.IsRetry,
134+
request.IsLastAttempt,
135+
request.CacheContext.SourceCacheContext.SessionId,
136+
log,
137+
lockedToken);
138+
139+
using (var throttledResponse = await throttledResponseFactory())
148140
{
149-
// Ignore reading and caching the empty stream.
150-
var httpSourceResult = new HttpSourceResult(HttpSourceResultStatus.NoContent);
151-
152-
return await processAsync(httpSourceResult);
153-
}
141+
if (request.IgnoreNotFounds && throttledResponse.Response.StatusCode == HttpStatusCode.NotFound)
142+
{
143+
var httpSourceResult = new HttpSourceResult(HttpSourceResultStatus.NotFound);
154144

155-
throttledResponse.Response.EnsureSuccessStatusCode();
145+
return await processAsync(httpSourceResult);
146+
}
156147

157-
if (!request.CacheContext.DirectDownload)
158-
{
159-
await HttpCacheUtility.CreateCacheFileAsync(
160-
cacheResult,
161-
throttledResponse.Response,
162-
request.EnsureValidContents,
163-
lockedToken);
164-
165-
using (var httpSourceResult = new HttpSourceResult(
166-
HttpSourceResultStatus.OpenedFromDisk,
167-
cacheResult.CacheFile,
168-
cacheResult.Stream))
148+
if (throttledResponse.Response.StatusCode == HttpStatusCode.NoContent)
169149
{
150+
// Ignore reading and caching the empty stream.
151+
var httpSourceResult = new HttpSourceResult(HttpSourceResultStatus.NoContent);
152+
170153
return await processAsync(httpSourceResult);
171154
}
172-
}
173-
else
174-
{
175-
// Note that we do not execute the content validator on the response stream when skipping
176-
// the cache. We cannot seek on the network stream and it is not valuable to download the
177-
// content twice just to validate the first time (considering that the second download could
178-
// be different from the first thus rendering the first validation meaningless).
155+
156+
throttledResponse.Response.EnsureSuccessStatusCode();
157+
158+
if (!request.CacheContext.DirectDownload)
159+
{
160+
await HttpCacheUtility.CreateCacheFileAsync(
161+
cacheResult,
162+
throttledResponse.Response,
163+
request.EnsureValidContents,
164+
lockedToken);
165+
166+
using (var httpSourceResult = new HttpSourceResult(
167+
HttpSourceResultStatus.OpenedFromDisk,
168+
cacheResult.CacheFile,
169+
cacheResult.Stream))
170+
{
171+
return await processAsync(httpSourceResult);
172+
}
173+
}
174+
else
175+
{
176+
// Note that we do not execute the content validator on the response stream when skipping
177+
// the cache. We cannot seek on the network stream and it is not valuable to download the
178+
// content twice just to validate the first time (considering that the second download could
179+
// be different from the first thus rendering the first validation meaningless).
179180
#if NETCOREAPP2_0_OR_GREATER
180181

181-
using (var stream = await throttledResponse.Response.Content.ReadAsStreamAsync(lockedToken))
182+
using (var stream = await throttledResponse.Response.Content.ReadAsStreamAsync(lockedToken))
182183
#else
183-
using (var stream = await throttledResponse.Response.Content.ReadAsStreamAsync())
184+
using (var stream = await throttledResponse.Response.Content.ReadAsStreamAsync())
184185
#endif
185-
using (var httpSourceResult = new HttpSourceResult(
186-
HttpSourceResultStatus.OpenedFromNetwork,
187-
cacheFileName: null,
188-
stream: stream))
189-
{
190-
return await processAsync(httpSourceResult);
186+
using (var httpSourceResult = new HttpSourceResult(
187+
HttpSourceResultStatus.OpenedFromNetwork,
188+
cacheFileName: null,
189+
stream: stream))
190+
{
191+
return await processAsync(httpSourceResult);
192+
}
191193
}
192194
}
193195
}
196+
finally
197+
{
198+
if (cacheResult.Stream != null)
199+
{
200+
cacheResult.Stream.Dispose();
201+
}
202+
}
194203
},
195204
token: token);
196205
}

0 commit comments

Comments
 (0)