Skip to content

Commit 6dd6a8e

Browse files
joelverhagenjoelverhagen
authored
Fix deadlock when reading packages from the drop folder (#30)
Fix potential race condition when two of the same packages are added at the same time Make allowOverrideExistingPackageOnPush default to false in repository. It is already defaults to false in the configuration.
1 parent 03d692c commit 6dd6a8e

5 files changed

Lines changed: 209 additions & 45 deletions

File tree

‎src/NuGet.Server.Core/Infrastructure/IServerPackageCache.cs‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,8 @@ public interface IServerPackageCache
1414
{
1515
bool IsEmpty();
1616

17+
bool Exists(string id, SemanticVersion version);
18+
1719
IEnumerable<ServerPackage> GetAll();
1820

1921
void Add(ServerPackage entity);

‎src/NuGet.Server.Core/Infrastructure/ServerPackageCache.cs‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -87,6 +87,19 @@ public bool IsEmpty()
8787
}
8888
}
8989

90+
public bool Exists(string id, SemanticVersion version)
91+
{
92+
_syncLock.EnterReadLock();
93+
try
94+
{
95+
return _packages.Any(p => IsMatch(p, id, version));
96+
}
97+
finally
98+
{
99+
_syncLock.ExitReadLock();
100+
}
101+
}
102+
90103
public IEnumerable<ServerPackage> GetAll()
91104
{
92105
_syncLock.EnterReadLock();

‎src/NuGet.Server.Core/Infrastructure/ServerPackageRepository.cs‎

Lines changed: 48 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -96,7 +96,7 @@ internal ServerPackageRepository(
9696
public string Source => _fileSystem.Root;
9797

9898
private bool AllowOverrideExistingPackageOnPush =>
99-
_settingsProvider.GetBoolSetting("allowOverrideExistingPackageOnPush", true);
99+
_settingsProvider.GetBoolSetting("allowOverrideExistingPackageOnPush", false);
100100

101101
private bool IgnoreSymbolsPackages =>
102102
_settingsProvider.GetBoolSetting("ignoreSymbolsPackages", false);
@@ -123,26 +123,7 @@ public async Task<IEnumerable<IServerPackage>> GetPackagesAsync(
123123
ClientCompatibility compatibility,
124124
CancellationToken token)
125125
{
126-
/*
127-
* We rebuild the package storage under either of two conditions:
128-
*
129-
* 1. If the "needs rebuild" flag is set to true. This is initially the case when the repository is
130-
* instantiated, if a non-package drop file system event occurred (e.g. a file deletion), or if the
131-
* cache was manually cleared.
132-
*
133-
* 2. If the store has no packages at all. This is so we pick up initial packages as quickly as
134-
* possible.
135-
*/
136-
if (_needsRebuild || _serverPackageCache.IsEmpty())
137-
{
138-
using (await LockAndSuppressFileSystemWatcherAsync(token))
139-
{
140-
if (_needsRebuild || _serverPackageCache.IsEmpty())
141-
{
142-
await RebuildPackageStoreWithoutLockingAsync(token);
143-
}
144-
}
145-
}
126+
await RebuildIfNeededAsync(shouldLock: true, token: token);
146127

147128
// First time we come here, attach the file system watcher.
148129
if (_fileSystemWatcher == null &&
@@ -158,7 +139,7 @@ public async Task<IEnumerable<IServerPackage>> GetPackagesAsync(
158139
{
159140
SetupBackgroundJobs();
160141
}
161-
142+
162143
var cache = _serverPackageCache.GetAll();
163144

164145
if (!compatibility.AllowSemVer2)
@@ -169,6 +150,38 @@ public async Task<IEnumerable<IServerPackage>> GetPackagesAsync(
169150
return cache;
170151
}
171152

153+
private async Task RebuildIfNeededAsync(bool shouldLock, CancellationToken token)
154+
{
155+
/*
156+
* We rebuild the package storage under either of two conditions:
157+
*
158+
* 1. If the "needs rebuild" flag is set to true. This is initially the case when the repository is
159+
* instantiated, if a non-package drop file system event occurred (e.g. a file deletion), or if the
160+
* cache was manually cleared.
161+
*
162+
* 2. If the store has no packages at all. This is so we pick up initial packages as quickly as
163+
* possible.
164+
*/
165+
if (_needsRebuild || _serverPackageCache.IsEmpty())
166+
{
167+
if (shouldLock)
168+
{
169+
using (await LockAndSuppressFileSystemWatcherAsync(token))
170+
{
171+
// Check the flags again, just in case another thread already did this work.
172+
if (_needsRebuild || _serverPackageCache.IsEmpty())
173+
{
174+
await RebuildPackageStoreWithoutLockingAsync(token);
175+
}
176+
}
177+
}
178+
else
179+
{
180+
await RebuildPackageStoreWithoutLockingAsync(token);
181+
}
182+
}
183+
}
184+
172185
public async Task<IEnumerable<IServerPackage>> SearchAsync(
173186
string searchTerm,
174187
IEnumerable<string> targetFrameworks,
@@ -202,18 +215,20 @@ public async Task<IEnumerable<IServerPackage>> SearchAsync(
202215
return packages;
203216
}
204217

205-
private async Task AddPackagesFromDropFolderAsync(CancellationToken token)
218+
internal async Task AddPackagesFromDropFolderAsync(CancellationToken token)
206219
{
207220
using (await LockAndSuppressFileSystemWatcherAsync(token))
208221
{
209-
await AddPackagesFromDropFolderWithoutLockingAsync(token);
222+
await RebuildIfNeededAsync(shouldLock: false, token: token);
223+
224+
AddPackagesFromDropFolderWithoutLocking();
210225
}
211226
}
212227

213228
/// <summary>
214229
/// This method requires <see cref="LockAndSuppressFileSystemWatcherAsync(CancellationToken)"/>.
215230
/// </summary>
216-
private async Task AddPackagesFromDropFolderWithoutLockingAsync(CancellationToken token)
231+
private void AddPackagesFromDropFolderWithoutLocking()
217232
{
218233
_logger.Log(LogLevel.Info, "Start adding packages from drop folder.");
219234

@@ -228,7 +243,7 @@ private async Task AddPackagesFromDropFolderWithoutLockingAsync(CancellationToke
228243
// Create package
229244
var package = new OptimizedZipPackage(_fileSystem, packageFile);
230245

231-
if (!await CanPackageBeAddedAsync(package, shouldThrow: false, token: token))
246+
if (!CanPackageBeAddedWithoutLocking(package, shouldThrow: false))
232247
{
233248
continue;
234249
}
@@ -275,10 +290,12 @@ public async Task AddPackageAsync(IPackage package, CancellationToken token)
275290
{
276291
_logger.Log(LogLevel.Info, "Start adding package {0} {1}.", package.Id, package.Version);
277292

278-
await CanPackageBeAddedAsync(package, shouldThrow: true, token: token);
279-
280293
using (await LockAndSuppressFileSystemWatcherAsync(token))
281294
{
295+
await RebuildIfNeededAsync(shouldLock: false, token: token);
296+
297+
CanPackageBeAddedWithoutLocking(package, shouldThrow: true);
298+
282299
// Add the package to the file system store.
283300
var serverPackage = _serverPackageStore.Add(
284301
package,
@@ -291,7 +308,7 @@ public async Task AddPackageAsync(IPackage package, CancellationToken token)
291308
}
292309
}
293310

294-
private async Task<bool> CanPackageBeAddedAsync(IPackage package, bool shouldThrow, CancellationToken token)
311+
private bool CanPackageBeAddedWithoutLocking(IPackage package, bool shouldThrow)
295312
{
296313
if (IgnoreSymbolsPackages && package.IsSymbolsPackage())
297314
{
@@ -309,7 +326,7 @@ private async Task<bool> CanPackageBeAddedAsync(IPackage package, bool shouldThr
309326

310327
// Does the package already exist?
311328
if (!AllowOverrideExistingPackageOnPush &&
312-
await this.ExistsAsync(package.Id, package.Version, token))
329+
_serverPackageCache.Exists(package.Id, package.Version))
313330
{
314331
var message = string.Format(Strings.Error_PackageAlreadyExists, package);
315332

@@ -414,7 +431,7 @@ private async Task RebuildPackageStoreWithoutLockingAsync(CancellationToken toke
414431
_serverPackageCache.AddRange(packages);
415432

416433
// Add packages from drop folder
417-
await AddPackagesFromDropFolderWithoutLockingAsync(token);
434+
AddPackagesFromDropFolderWithoutLocking();
418435

419436
// Persist
420437
_serverPackageCache.PersistIfDirty();

‎test/NuGet.Server.Core.Tests/ServerPackageCacheTest.cs‎

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -197,5 +197,59 @@ public void Remove_NoOpsWhenPackageDoesNotExist()
197197
Assert.Equal(PackageId, package.Id);
198198
Assert.Equal(PackageVersion, package.Version);
199199
}
200+
201+
[Fact]
202+
public void Exists_IsCaseInsensitive()
203+
{
204+
// Arrange
205+
var fileSystem = new Mock<IFileSystem>();
206+
fileSystem
207+
.Setup(x => x.FileExists(CacheFileName))
208+
.Returns(false);
209+
var target = new ServerPackageCache(fileSystem.Object, CacheFileName);
210+
target.Add(new ServerPackage { Id = "NuGet.Versioning", Version = new SemanticVersion("3.5.0-beta2") });
211+
212+
// Act
213+
var actual = target.Exists("nuget.versioning", new SemanticVersion("3.5.0-BETA2"));
214+
215+
// Assert
216+
Assert.True(actual);
217+
}
218+
219+
[Fact]
220+
public void Exists_ReturnsFalseWhenPackageDoesNotExist()
221+
{
222+
// Arrange
223+
var fileSystem = new Mock<IFileSystem>();
224+
fileSystem
225+
.Setup(x => x.FileExists(CacheFileName))
226+
.Returns(false);
227+
var target = new ServerPackageCache(fileSystem.Object, CacheFileName);
228+
target.Add(new ServerPackage { Id = "NuGet.Versioning", Version = new SemanticVersion("3.5.0-beta2") });
229+
230+
// Act
231+
var actual = target.Exists("NuGet.Frameworks", new SemanticVersion("3.5.0-beta2"));
232+
233+
// Assert
234+
Assert.False(actual);
235+
}
236+
237+
[Fact]
238+
public void Exists_ReturnsTrueWhenPackageExists()
239+
{
240+
// Arrange
241+
var fileSystem = new Mock<IFileSystem>();
242+
fileSystem
243+
.Setup(x => x.FileExists(CacheFileName))
244+
.Returns(false);
245+
var target = new ServerPackageCache(fileSystem.Object, CacheFileName);
246+
target.Add(new ServerPackage { Id = "NuGet.Versioning", Version = new SemanticVersion("3.5.0-beta2") });
247+
248+
// Act
249+
var actual = target.Exists("NuGet.Versioning", new SemanticVersion("3.5.0-beta2"));
250+
251+
// Assert
252+
Assert.True(actual);
253+
}
200254
}
201255
}

‎test/NuGet.Server.Core.Tests/ServerPackageRepositoryTest.cs‎

Lines changed: 92 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -54,8 +54,10 @@ private async Task<ServerPackageRepository> CreateServerPackageRepositoryWithSem
5454
});
5555
}
5656

57-
[Fact]
58-
public async Task ServerPackageRepositoryAddsPackagesFromDropFolderOnStart()
57+
[Theory]
58+
[InlineData(true)]
59+
[InlineData(false)]
60+
public async Task ServerPackageRepositoryAddsPackagesFromDropFolderOnStart(bool allowOverride)
5961
{
6062
using (var temporaryDirectory = new TemporaryDirectory())
6163
{
@@ -78,7 +80,17 @@ public async Task ServerPackageRepositoryAddsPackagesFromDropFolderOnStart()
7880
}
7981
}
8082

81-
var serverRepository = await CreateServerPackageRepositoryAsync(temporaryDirectory.Path);
83+
var serverRepository = await CreateServerPackageRepositoryAsync(
84+
temporaryDirectory.Path,
85+
getSetting: (key, defaultValue) =>
86+
{
87+
if (key == "allowOverrideExistingPackageOnPush")
88+
{
89+
return allowOverride;
90+
}
91+
92+
return defaultValue;
93+
});
8294

8395
// Act
8496
var packages = await serverRepository.GetPackagesAsync(ClientCompatibility.Max, Token);
@@ -135,17 +147,12 @@ public async Task ServerPackageRepositoryRemovePackage()
135147
}
136148

137149
[Fact]
138-
public async Task ServerPackageRepositoryNeedsRebuild()
150+
public async Task ServerPackageRepositoryNeedsRebuildIsHandledWhenAddingAfterClear()
139151
{
140152
using (var temporaryDirectory = new TemporaryDirectory())
141153
{
142154
// Arrange
143-
var serverRepository = await CreateServerPackageRepositoryAsync(temporaryDirectory.Path,
144-
repository =>
145-
{
146-
repository.AddPackage(CreatePackage("test", "1.0"));
147-
repository.AddPackage(CreatePackage("test", "1.1"));
148-
});
155+
var serverRepository = await CreateServerPackageRepositoryAsync(temporaryDirectory.Path);
149156

150157
// Act
151158
await serverRepository.ClearCacheAsync(Token);
@@ -155,10 +162,81 @@ public async Task ServerPackageRepositoryNeedsRebuild()
155162
// Assert
156163
packages = packages.OrderBy(p => p.Version);
157164

158-
Assert.Equal(3, packages.Count());
159-
Assert.Equal(new SemanticVersion("1.0.0"), packages.ElementAt(0).Version);
160-
Assert.Equal(new SemanticVersion("1.1.0"), packages.ElementAt(1).Version);
161-
Assert.Equal(new SemanticVersion("1.2.0"), packages.ElementAt(2).Version);
165+
Assert.Equal(1, packages.Count());
166+
Assert.Equal(new SemanticVersion("1.2.0"), packages.ElementAt(0).Version);
167+
}
168+
}
169+
170+
[Theory]
171+
[InlineData(false)]
172+
[InlineData(true)]
173+
public async Task ServerPackageRepository_DuplicateAddAfterClearObservesOverrideOption(bool allowOverride)
174+
{
175+
using (var temporaryDirectory = new TemporaryDirectory())
176+
{
177+
// Arrange
178+
var serverRepository = await CreateServerPackageRepositoryAsync(
179+
temporaryDirectory.Path,
180+
getSetting: (key, defaultValue) =>
181+
{
182+
if (key == "allowOverrideExistingPackageOnPush")
183+
{
184+
return allowOverride;
185+
}
186+
187+
return defaultValue;
188+
});
189+
190+
await serverRepository.AddPackageAsync(CreatePackage("test", "1.2"), Token);
191+
await serverRepository.ClearCacheAsync(Token);
192+
193+
// Act & Assert
194+
if (allowOverride)
195+
{
196+
await serverRepository.AddPackageAsync(CreatePackage("test", "1.2"), Token);
197+
}
198+
else
199+
{
200+
await Assert.ThrowsAsync<InvalidOperationException>(async () =>
201+
await serverRepository.AddPackageAsync(CreatePackage("test", "1.2"), Token));
202+
}
203+
}
204+
}
205+
206+
[Theory]
207+
[InlineData(false)]
208+
[InlineData(true)]
209+
public async Task ServerPackageRepository_DuplicateInDropFolderAfterClearObservesOverrideOption(bool allowOverride)
210+
{
211+
using (var temporaryDirectory = new TemporaryDirectory())
212+
{
213+
// Arrange
214+
var serverRepository = await CreateServerPackageRepositoryAsync(
215+
temporaryDirectory.Path,
216+
getSetting: (key, defaultValue) =>
217+
{
218+
if (key == "allowOverrideExistingPackageOnPush")
219+
{
220+
return allowOverride;
221+
}
222+
223+
return defaultValue;
224+
});
225+
226+
await serverRepository.AddPackageAsync(CreatePackage("test", "1.2"), Token);
227+
var existingPackage = await serverRepository.FindPackageAsync(
228+
"test",
229+
new SemanticVersion("1.2"),
230+
Token);
231+
var dropFolderPackagePath = Path.Combine(temporaryDirectory, "test.nupkg");
232+
await serverRepository.ClearCacheAsync(Token);
233+
234+
// Act
235+
File.Copy(existingPackage.FullPath, dropFolderPackagePath);
236+
await serverRepository.AddPackagesFromDropFolderAsync(Token);
237+
238+
// Assert
239+
Assert.NotEqual(allowOverride, File.Exists(dropFolderPackagePath));
162240
}
163241
}
164242

0 commit comments

Comments
 (0)