Skip to content

Commit

Permalink
Fix collection modified exception which causes rare HTTP 500s (#5830)
Browse files Browse the repository at this point in the history 
Add unit tests to verify casing changes and repro of bug
Use ConcurrentDictionary instead of Dictionary
Address #5779
  • Loading branch information
@joelverhagen
joelverhagen committed Apr 20, 2018
1 parent fcd6626 commit f120d52
Show file tree
Hide file tree
Showing 3 changed files with 270 additions and 62 deletions.
140 changes: 78 additions & 62 deletions src/NuGetGallery/Services/CloudDownloadCountService.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System;
using System.Collections.Concurrent;
using System.Collections.Generic;
using System.IO;
using System.Linq;
Expand All @@ -27,9 +28,8 @@ public class CloudDownloadCountService : IDownloadCountService
private readonly object _refreshLock = new object();
private bool _isRefreshing;

private readonly IDictionary<string, IDictionary<string, int>> _downloadCounts = new Dictionary<string, IDictionary<string, int>>(StringComparer.OrdinalIgnoreCase);

public DateTime LastRefresh { get; protected set; }
private readonly ConcurrentDictionary<string, ConcurrentDictionary<string, int>> _downloadCounts
= new ConcurrentDictionary<string, ConcurrentDictionary<string, int>>(StringComparer.OrdinalIgnoreCase);

public CloudDownloadCountService(ITelemetryClient telemetryClient, string connectionString, bool readAccessGeoRedundant)
{
Expand All @@ -38,19 +38,17 @@ public CloudDownloadCountService(ITelemetryClient telemetryClient, string connec
_connectionString = connectionString;
_readAccessGeoRedundant = readAccessGeoRedundant;
}

public bool TryGetDownloadCountForPackageRegistration(string id, out int downloadCount)
{
if (string.IsNullOrEmpty(id))
{
throw new ArgumentNullException(nameof(id));
}

id = id.ToLowerInvariant();

if (_downloadCounts.ContainsKey(id))
if (_downloadCounts.TryGetValue(id, out var versions))
{
downloadCount = _downloadCounts[id].Sum(kvp => kvp.Value);
downloadCount = CalculateSum(versions);
return true;
}

Expand All @@ -70,16 +68,10 @@ public bool TryGetDownloadCountForPackage(string id, string version, out int dow
throw new ArgumentNullException(nameof(version));
}

id = id.ToLowerInvariant();
version = version.ToLowerInvariant();

if (_downloadCounts.ContainsKey(id))
if (_downloadCounts.TryGetValue(id, out var versions)
&& versions.TryGetValue(version, out downloadCount))
{
if (_downloadCounts[id].ContainsKey(version))
{
downloadCount = _downloadCounts[id][version];
return true;
}
return true;
}

downloadCount = 0;
Expand Down Expand Up @@ -127,78 +119,102 @@ public void Refresh()
finally
{
_isRefreshing = false;
LastRefresh = DateTime.UtcNow;
}
}
}

/// <summary>
/// This method is added for unit testing purposes.
/// </summary>
protected virtual int CalculateSum(ConcurrentDictionary<string, int> versions)
{
return versions.Sum(kvp => kvp.Value);
}

/// <summary>
/// This method is added for unit testing purposes. It can return a null stream if the blob does not exist
/// and assumes the caller will properly dispose of the returned stream.
/// </summary>
protected virtual Stream GetBlobStream()
{
var blob = GetBlobReference();
if (blob == null)
{
return null;
}

return blob.OpenRead();
}

private void RefreshCore()
{
try
{
var blob = GetBlobReference();
if (blob == null)
{
return;
}

// The data in downloads.v1.json will be an array of Package records - which has Id, Array of Versions and download count.
// Sample.json : [["AutofacContrib.NSubstitute",["2.4.3.700",406],["2.5.0",137]],["Assman.Core",["2.0.7",138]]....
using (var jsonReader = new JsonTextReader(new StreamReader(blob.OpenRead())))
using (var blobStream = GetBlobStream())
{
try
if (blobStream == null)
{
jsonReader.Read();
return;
}

while (jsonReader.Read())
using (var jsonReader = new JsonTextReader(new StreamReader(blobStream)))
{
try
{
try
jsonReader.Read();

while (jsonReader.Read())
{
if (jsonReader.TokenType == JsonToken.StartArray)
try
{
JToken record = JToken.ReadFrom(jsonReader);
string id = record[0].ToString().ToLowerInvariant();

// The second entry in each record should be an array of versions, if not move on to next entry.
// This is a check to safe guard against invalid entries.
if (record.Count() == 2 && record[1].Type != JTokenType.Array)
if (jsonReader.TokenType == JsonToken.StartArray)
{
continue;
}
JToken record = JToken.ReadFrom(jsonReader);
string id = record[0].ToString().ToLowerInvariant();

if (!_downloadCounts.ContainsKey(id))
{
_downloadCounts.Add(id, new Dictionary<string, int>(StringComparer.OrdinalIgnoreCase));
}
var versions = _downloadCounts[id];
// The second entry in each record should be an array of versions, if not move on to next entry.
// This is a check to safe guard against invalid entries.
if (record.Count() == 2 && record[1].Type != JTokenType.Array)
{
continue;
}

foreach (JToken token in record)
{
if (token != null && token.Count() == 2)
var versions = _downloadCounts.GetOrAdd(
id,
_ => new ConcurrentDictionary<string, int>(StringComparer.OrdinalIgnoreCase));

foreach (JToken token in record)
{
string version = token[0].ToString().ToLowerInvariant();
versions[version] = token[1].ToObject<int>();
if (token != null && token.Count() == 2)
{
var version = token[0].ToString();
var downloadCount = token[1].ToObject<int>();

versions.AddOrSet(version, downloadCount);
}
}
}
}
}
catch (JsonReaderException ex)
{
_telemetryClient.TrackException(ex, new Dictionary<string, string>
catch (JsonReaderException ex)
{
{ "Origin", TelemetryOriginForRefreshMethod },
{ "AdditionalInfo", "Invalid entry found in downloads.v1.json." }
});
_telemetryClient.TrackException(ex, new Dictionary<string, string>
{
{ "Origin", TelemetryOriginForRefreshMethod },
{ "AdditionalInfo", "Invalid entry found in downloads.v1.json." }
});
}
}
}
}
catch (JsonReaderException ex)
{
_telemetryClient.TrackException(ex, new Dictionary<string, string>
catch (JsonReaderException ex)
{
{ "Origin", TelemetryOriginForRefreshMethod },
{ "AdditionalInfo", "Data present in downloads.v1.json is invalid. Couldn't get download data." }
});
_telemetryClient.TrackException(ex, new Dictionary<string, string>
{
{ "Origin", TelemetryOriginForRefreshMethod },
{ "AdditionalInfo", "Data present in downloads.v1.json is invalid. Couldn't get download data." }
});
}
}
}
}
Expand Down
1 change: 1 addition & 0 deletions tests/NuGetGallery.Facts/NuGetGallery.Facts.csproj
View file
Original file line number Diff line number Diff line change
Expand Up @@ -445,6 +445,7 @@
<Compile Include="Security\RequireMinProtocolVersionForPushPolicyFacts.cs" />
<Compile Include="Security\RequireOrganizationTenantPolicyFacts.cs" />
<Compile Include="Services\ActionRequiringEntityPermissionsFacts.cs" />
<Compile Include="Services\CloudDownloadCountServiceFacts.cs" />
<Compile Include="Services\DeleteAccountServiceFacts.cs" />
<Compile Include="Services\JsonStatisticsServiceFacts.cs" />
<Compile Include="Services\ContentObjectServiceFacts.cs" />
Expand Down
Loading

0 comments on commit f120d52

Please sign in to comment.