Skip to content

Commit dd91e10

Browse files
joelverhagenjoelverhagen
committed
Fix potential race condition with etags returned by GetFileReferenceAsync (#6500)
Progress on #6442 Address #6502
1 parent 3b552d9 commit dd91e10

3 files changed

Lines changed: 108 additions & 13 deletions

File tree

‎src/NuGetGallery.Core/Services/CloudBlobCoreFileStorageService.cs‎

Lines changed: 8 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -105,11 +105,7 @@ public async Task<IFileReference> GetFileReferenceAsync(string folderName, strin
105105
}
106106
else if (result.StatusCode == HttpStatusCode.OK)
107107
{
108-
if (await blob.ExistsAsync())
109-
{
110-
await blob.FetchAttributesAsync();
111-
}
112-
return CloudFileReference.Modified(blob, result.Data);
108+
return CloudFileReference.Modified(result.Data, result.ETag);
113109
}
114110
else
115111
{
@@ -505,11 +501,11 @@ await blob.DownloadToStreamAsync(
505501

506502
if (ex.RequestInformation.HttpStatusCode == (int)HttpStatusCode.NotModified)
507503
{
508-
return new StorageResult(HttpStatusCode.NotModified, null);
504+
return new StorageResult(HttpStatusCode.NotModified, null, blob.ETag);
509505
}
510506
else if (ex.RequestInformation.ExtendedErrorInformation?.ErrorCode == BlobErrorCodeStrings.BlobNotFound)
511507
{
512-
return new StorageResult(HttpStatusCode.NotFound, null);
508+
return new StorageResult(HttpStatusCode.NotFound, null, blob.ETag);
513509
}
514510

515511
throw;
@@ -522,14 +518,14 @@ await blob.DownloadToStreamAsync(
522518

523519
if (ex.ErrorCode == BlobErrorCodeStrings.BlobNotFound)
524520
{
525-
return new StorageResult(HttpStatusCode.NotFound, null);
521+
return new StorageResult(HttpStatusCode.NotFound, null, blob.ETag);
526522
}
527523

528524
throw;
529525
}
530526

531527
stream.Position = 0;
532-
return new StorageResult(HttpStatusCode.OK, stream);
528+
return new StorageResult(HttpStatusCode.OK, stream, blob.ETag);
533529
}
534530

535531
private static string GetContentType(string folderName)
@@ -607,11 +603,13 @@ private struct StorageResult
607603
{
608604
public HttpStatusCode StatusCode { get; }
609605
public Stream Data { get; }
606+
public string ETag { get; }
610607

611-
public StorageResult(HttpStatusCode statusCode, Stream data)
608+
public StorageResult(HttpStatusCode statusCode, Stream data, string etag)
612609
{
613610
StatusCode = statusCode;
614611
Data = data;
612+
ETag = etag;
615613
}
616614
}
617615
}

‎src/NuGetGallery.Core/Services/CloudFileReference.cs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -26,9 +26,9 @@ public static CloudFileReference NotModified(string contentId)
2626
return new CloudFileReference(null, contentId);
2727
}
2828

29-
public static CloudFileReference Modified(ISimpleCloudBlob blob, Stream data)
29+
public static CloudFileReference Modified(Stream data, string contentId)
3030
{
31-
return new CloudFileReference(data, blob.ETag);
31+
return new CloudFileReference(data, contentId);
3232
}
3333
}
3434
}

‎tests/NuGetGallery.Core.Facts/Services/CloudBlobCoreFileStorageServiceIntegrationTests.cs‎

Lines changed: 98 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,15 +2,18 @@
22
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
33

44
using System;
5+
using System.Collections.Concurrent;
56
using System.IO;
67
using System.Text;
8+
using System.Threading;
79
using System.Threading.Tasks;
810
using Microsoft.WindowsAzure.Storage;
911
using Microsoft.WindowsAzure.Storage.Auth;
1012
using Microsoft.WindowsAzure.Storage.Blob;
1113
using Moq;
1214
using NuGetGallery.Diagnostics;
1315
using Xunit;
16+
using Xunit.Abstractions;
1417

1518
namespace NuGetGallery
1619
{
@@ -26,6 +29,7 @@ private delegate Task CopyAsync(
2629
string destFileName);
2730

2831
private readonly BlobStorageFixture _fixture;
32+
private readonly ITestOutputHelper _output;
2933
private readonly string _testId;
3034
private readonly string _prefixA;
3135
private readonly string _prefixB;
@@ -36,9 +40,10 @@ private delegate Task CopyAsync(
3640
private readonly CloudBlobCoreFileStorageService _targetA;
3741
private readonly CloudBlobCoreFileStorageService _targetB;
3842

39-
public CloudBlobCoreFileStorageServiceIntegrationTests(BlobStorageFixture fixture)
43+
public CloudBlobCoreFileStorageServiceIntegrationTests(BlobStorageFixture fixture, ITestOutputHelper output)
4044
{
4145
_fixture = fixture ?? throw new ArgumentNullException(nameof(fixture));
46+
_output = output ?? throw new ArgumentNullException(nameof(output));
4247
_testId = Guid.NewGuid().ToString();
4348
_prefixA = $"{_fixture.PrefixA}/{_testId}";
4449
_prefixB = $"{_fixture.PrefixB}/{_testId}";
@@ -53,6 +58,98 @@ public CloudBlobCoreFileStorageServiceIntegrationTests(BlobStorageFixture fixtur
5358
_targetB = new CloudBlobCoreFileStorageService(_clientB, Mock.Of<IDiagnosticsService>());
5459
}
5560

61+
[BlobStorageFact]
62+
public async Task ReturnsCurrentETagForIfMatch()
63+
{
64+
// Arrange
65+
var folderName = CoreConstants.ValidationFolderName;
66+
var fileName = _prefixA;
67+
await _targetA.SaveFileAsync(folderName, fileName, new MemoryStream(new byte[0]));
68+
var initialReference = await _targetA.GetFileReferenceAsync(folderName, fileName);
69+
initialReference.OpenRead().Dispose();
70+
71+
// Act
72+
var reference = await _targetA.GetFileReferenceAsync(folderName, fileName, initialReference.ContentId);
73+
74+
// Assert
75+
Assert.NotNull(reference);
76+
Assert.Null(reference.OpenRead());
77+
Assert.Equal(initialReference.ContentId, reference.ContentId);
78+
}
79+
80+
[BlobStorageFact]
81+
public async Task ReturnsNullForMissingBlob()
82+
{
83+
// Arrange
84+
var folderName = CoreConstants.ValidationFolderName;
85+
var fileName = _prefixA;
86+
87+
// Act
88+
var reference = await _targetA.GetFileReferenceAsync(folderName, fileName);
89+
90+
// Assert
91+
Assert.Null(reference);
92+
}
93+
94+
[BlobStorageFact]
95+
public async Task ReturnsTheETagMatchingTheContent()
96+
{
97+
// Arrange
98+
var folderName = CoreConstants.ValidationFolderName;
99+
var fileName = _prefixA;
100+
var contentToETag = new ConcurrentDictionary<string, string>();
101+
var iterations = 20;
102+
var cts = new CancellationTokenSource();
103+
104+
Func<Task> update = async () =>
105+
{
106+
var container = _blobClientA.GetContainerReference(folderName);
107+
for (var i = 1; i <= iterations && !cts.IsCancellationRequested; i++)
108+
{
109+
var blob = container.GetBlockBlobReference(fileName);
110+
var content = i.ToString();
111+
await blob.UploadTextAsync(content);
112+
contentToETag[content] = blob.Properties.ETag;
113+
_output.WriteLine($"Content '{content}' should have etag '{blob.Properties.ETag}'.");
114+
}
115+
};
116+
117+
Func<Task> check = async () =>
118+
{
119+
string content = null;
120+
while (content != iterations.ToString())
121+
{
122+
var fileReference = await _targetA.GetFileReferenceAsync(folderName, fileName);
123+
if (fileReference == null)
124+
{
125+
continue;
126+
}
127+
128+
using (var stream = fileReference.OpenRead())
129+
using (var streamReader = new StreamReader(stream))
130+
{
131+
content = await streamReader.ReadToEndAsync();
132+
if (contentToETag.TryGetValue(content, out var expectedETag))
133+
{
134+
_output.WriteLine($"Content '{content}' has etag '{fileReference.ContentId}'.");
135+
if (expectedETag != fileReference.ContentId)
136+
{
137+
cts.Cancel();
138+
}
139+
140+
Assert.Equal(expectedETag, fileReference.ContentId);
141+
}
142+
}
143+
}
144+
};
145+
146+
// Act & Assert
147+
var updateTask = update();
148+
var checkTask = check();
149+
await checkTask;
150+
await updateTask;
151+
}
152+
56153
[BlobStorageFact]
57154
public async Task CanReadAndDeleteBlobUsingPrivilegedFileUri()
58155
{

0 commit comments

Comments
 (0)