-
Notifications
You must be signed in to change notification settings - Fork 696
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Addressing concurrency exceptions when incrementing the download count. #716
base: main
Are you sure you want to change the base?
Changes from 5 commits
e184f93
dda9037
c7bf4a5
f30b2bc
bc04655
8881ea3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,24 +11,28 @@ namespace BaGet.Core | |
public class PackageDatabase : IPackageDatabase | ||
{ | ||
private readonly IContext _context; | ||
private readonly Func<IContext> _newContext; | ||
|
||
public PackageDatabase(IContext context) | ||
public PackageDatabase(IContext context, Func<IContext> newContext) | ||
{ | ||
_context = context ?? throw new ArgumentNullException(nameof(context)); | ||
_newContext = newContext ?? throw new ArgumentNullException(nameof(newContext)); | ||
} | ||
|
||
public async Task<PackageAddResult> AddAsync(Package package, CancellationToken cancellationToken) | ||
{ | ||
using var context = _newContext(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This read as confusing with the field _context and this. Why is the field still required? Could it not always generate context? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agree and have updated this. Would like to change this everywhere to be honest. I don't like relying on DI to manage DbContext lifetime. |
||
|
||
try | ||
{ | ||
_context.Packages.Add(package); | ||
context.Packages.Add(package); | ||
|
||
await _context.SaveChangesAsync(cancellationToken); | ||
await context.SaveChangesAsync(cancellationToken); | ||
|
||
return PackageAddResult.Success; | ||
} | ||
catch (DbUpdateException e) | ||
when (_context.IsUniqueConstraintViolationException(e)) | ||
when (context.IsUniqueConstraintViolationException(e)) | ||
{ | ||
return PackageAddResult.PackageAlreadyExists; | ||
} | ||
|
@@ -99,12 +103,14 @@ public Task<bool> RelistPackageAsync(string id, NuGetVersion version, Cancellati | |
|
||
public async Task AddDownloadAsync(string id, NuGetVersion version, CancellationToken cancellationToken) | ||
{ | ||
await TryUpdatePackageAsync(id, version, p => p.Downloads += 1, cancellationToken); | ||
await TryUpdatePackageWithConcurrencyRetryAsync(id, version, p => p.Downloads += 1, cancellationToken); | ||
} | ||
|
||
public async Task<bool> HardDeletePackageAsync(string id, NuGetVersion version, CancellationToken cancellationToken) | ||
{ | ||
var package = await _context.Packages | ||
using var context = _newContext(); | ||
|
||
var package = await context.Packages | ||
.Where(p => p.Id == id) | ||
.Where(p => p.NormalizedVersionString == version.ToNormalizedString()) | ||
.Include(p => p.Dependencies) | ||
|
@@ -116,8 +122,8 @@ public async Task<bool> HardDeletePackageAsync(string id, NuGetVersion version, | |
return false; | ||
} | ||
|
||
_context.Packages.Remove(package); | ||
await _context.SaveChangesAsync(cancellationToken); | ||
context.Packages.Remove(package); | ||
await context.SaveChangesAsync(cancellationToken); | ||
|
||
return true; | ||
} | ||
|
@@ -128,20 +134,48 @@ private async Task<bool> TryUpdatePackageAsync( | |
Action<Package> action, | ||
CancellationToken cancellationToken) | ||
{ | ||
var package = await _context.Packages | ||
using var context = _newContext(); | ||
|
||
var package = await context.Packages | ||
.Where(p => p.Id == id) | ||
.Where(p => p.NormalizedVersionString == version.ToNormalizedString()) | ||
.FirstOrDefaultAsync(); | ||
|
||
if (package != null) | ||
{ | ||
action(package); | ||
await _context.SaveChangesAsync(cancellationToken); | ||
await context.SaveChangesAsync(cancellationToken); | ||
|
||
return true; | ||
} | ||
|
||
return false; | ||
} | ||
|
||
private async Task<bool> TryUpdatePackageWithConcurrencyRetryAsync( | ||
string id, | ||
NuGetVersion version, | ||
Action<Package> action, | ||
CancellationToken cancellationToken) | ||
{ | ||
var attempts = 0; | ||
|
||
while (true) | ||
{ | ||
try | ||
{ | ||
return await TryUpdatePackageAsync(id, version, action, cancellationToken); | ||
} | ||
catch (DbUpdateConcurrencyException) | ||
{ | ||
attempts++; | ||
|
||
if (attempts >= 5) | ||
{ | ||
throw; | ||
} | ||
} | ||
} | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggest naming this context generator so it's clearer what it is?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer to name the factory closer to the idiomatic C# code, i.e.
as an analog to
I can can probably be convinced to rename to _createContext ;)
Let's be honest though, we all nab the fixes and then apply our own naming conventions here.