Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix for UpdateIsLatestAsync retry connection string #3632

Merged
merged 1 commit into from
Mar 8, 2017
Merged

Fix for UpdateIsLatestAsync retry connection string #3632

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 15 additions & 1 deletion src/NuGetGallery/Services/PackageService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
using NuGet.Packaging;
using NuGet.Versioning;
using NuGetGallery.Auditing;
using NuGetGallery.Configuration;
using NuGetGallery.Diagnostics;
using NuGetGallery.Packaging;

Expand All @@ -25,6 +26,7 @@ public class PackageService : IPackageService

private readonly IIndexingService _indexingService;
private readonly IEntitiesContext _entitiesContext;
private readonly IAppConfiguration _configuration;
private readonly IEntityRepository<PackageOwnerRequest> _packageOwnerRequestRepository;
private readonly IEntityRepository<PackageRegistration> _packageRegistrationRepository;
private readonly IEntityRepository<Package> _packageRepository;
Expand All @@ -37,6 +39,7 @@ public PackageService(
IEntityRepository<Package> packageRepository,
IEntityRepository<PackageOwnerRequest> packageOwnerRequestRepository,
IEntitiesContext entitiesContext,
IAppConfiguration configuration,
IDiagnosticsService diagnostics,
IIndexingService indexingService,
IPackageNamingConflictValidator packageNamingConflictValidator,
Expand All @@ -57,6 +60,16 @@ public PackageService(
throw new ArgumentNullException(nameof(packageOwnerRequestRepository));
}

if (entitiesContext == null)
{
throw new ArgumentNullException(nameof(entitiesContext));
}

if (configuration == null)
{
throw new ArgumentNullException(nameof(configuration));
}

if (indexingService == null)
{
throw new ArgumentNullException(nameof(indexingService));
Expand All @@ -76,6 +89,7 @@ public PackageService(
_packageRepository = packageRepository;
_packageOwnerRequestRepository = packageOwnerRequestRepository;
_entitiesContext = entitiesContext;
_configuration = configuration;
_indexingService = indexingService;
_packageNamingConflictValidator = packageNamingConflictValidator;
_auditingService = auditingService;
Expand Down Expand Up @@ -780,7 +794,7 @@ private Task<bool> TryUpdateIsLatestAsync(IEntitiesContext context, PackageRegis

protected internal virtual IEntitiesContext CreateNewEntitiesContext()
{
return new EntitiesContext();
return new EntitiesContext(_configuration.SqlConnectionString, readOnly: false);
}

public async Task UpdateIsLatestAsync(PackageRegistration packageRegistration)
Expand Down
7 changes: 7 additions & 0 deletions tests/NuGetGallery.Facts/Services/PackageServiceFacts.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
using NuGet.Packaging;
using NuGet.Versioning;
using NuGetGallery.Auditing;
using NuGetGallery.Configuration;
using NuGetGallery.Diagnostics;
using NuGetGallery.Framework;
using NuGetGallery.Packaging;
Expand Down Expand Up @@ -100,6 +101,7 @@ private static IPackageService CreateService(
Mock<IEntityRepository<Package>> packageRepository = null,
Mock<IEntityRepository<PackageOwnerRequest>> packageOwnerRequestRepo = null,
Mock<IEntitiesContext> entitiesContext = null,
Mock<IAppConfiguration> configuration = null,
Mock<IDiagnosticsService> diagnosticsService = null,
Mock<IIndexingService> indexingService = null,
IPackageNamingConflictValidator packageNamingConflictValidator = null,
Expand All @@ -111,6 +113,7 @@ private static IPackageService CreateService(
packageRepository,
packageOwnerRequestRepo,
entitiesContext,
configuration,
diagnosticsService,
indexingService,
packageNamingConflictValidator,
Expand All @@ -123,6 +126,7 @@ private static Mock<PackageService> CreateServiceMock(
Mock<IEntityRepository<Package>> packageRepository = null,
Mock<IEntityRepository<PackageOwnerRequest>> packageOwnerRequestRepo = null,
Mock<IEntitiesContext> entitiesContext = null,
Mock<IAppConfiguration> configuration = null,
Mock<IDiagnosticsService> diagnosticsService = null,
Mock<IIndexingService> indexingService = null,
IPackageNamingConflictValidator packageNamingConflictValidator = null,
Expand All @@ -138,6 +142,8 @@ private static Mock<PackageService> CreateServiceMock(
entitiesContext.Setup(m => m.GetDatabase()).Returns(dbContext.Object.Database);
entitiesContext.Setup(m => m.GetChangeTracker()).Returns(dbContext.Object.ChangeTracker);

configuration = configuration ?? new Mock<IAppConfiguration>();

diagnosticsService = diagnosticsService ?? new Mock<IDiagnosticsService>();
indexingService = indexingService ?? new Mock<IIndexingService>();
auditingService = auditingService ?? new TestAuditingService();
Expand All @@ -154,6 +160,7 @@ private static Mock<PackageService> CreateServiceMock(
packageRepository.Object,
packageOwnerRequestRepo.Object,
entitiesContext.Object,
configuration.Object,
diagnosticsService.Object,
indexingService.Object,
packageNamingConflictValidator,
Expand Down
3 changes: 3 additions & 0 deletions tests/NuGetGallery.Facts/Services/ReflowPackageServiceFacts.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
using System.Threading.Tasks;
using Moq;
using NuGet.Packaging;
using NuGetGallery.Configuration;
using NuGetGallery.Diagnostics;
using NuGetGallery.Framework;
using NuGetGallery.Packaging;
Expand Down Expand Up @@ -285,6 +286,7 @@ private static Mock<PackageService> SetupPackageService(Package package)
var packageOwnerRequestRepo = new Mock<IEntityRepository<PackageOwnerRequest>>();
var diagnosticsService = new Mock<IDiagnosticsService>();
var entitiesContext = new Mock<IEntitiesContext>();
var configuration = new Mock<IAppConfiguration>();
var indexingService = new Mock<IIndexingService>();
var packageNamingConflictValidator = new PackageNamingConflictValidator(
packageRegistrationRepository.Object,
Expand All @@ -296,6 +298,7 @@ private static Mock<PackageService> SetupPackageService(Package package)
packageRepository.Object,
packageOwnerRequestRepo.Object,
entitiesContext.Object,
configuration.Object,
diagnosticsService.Object,
indexingService.Object,
packageNamingConflictValidator,
Expand Down
15 changes: 12 additions & 3 deletions tests/NuGetGallery.FunctionalTests/ODataFeeds/V2FeedExtendedTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,12 +113,21 @@ public async Task PackageLatestSetCorrectlyOnConcurrentPushes()
"7.0.0-a", "7.0.0-b", "7.0.0", "7.0.1", "7.0.2-abc"
};

// push all and verify; ~15-20 concurrency conflicts seen in testing
// first push should not be concurrent to avoid conflict on creation of package registration.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens when there is a conflict on creation of the package registration? Is it an issue we should fix?

Copy link
Member Author

@chenriksson chenriksson Mar 7, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mainly did this so there wouldn't be lots of first chance exceptions for the conflict when debugging this test. Client does a retry, so I was still seeing the test pass.

I suppose the test could have failed or been flaky if the first push was done concurrently and there were a lot of conflicts which caused the client to exceed retry.

I don't think this is anything to fix here. The conflict I'm trying to reproduce is with UpdateIsLatest, not package registration creation.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To clarify, I agree that it's out of the scope of this PR, but I think it may be something to look into in the future.
If the same person pushes two separate versions of the same new package at once, there shouldn't be issues--we may want to consider adding some failure logic when the package registration commit fails.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough - agree, sounds like another useful functional test to have.

var concurrentTasks = new Task[packageVersions.Count];
for (int i = 0; i < concurrentTasks.Length; i++)
concurrentTasks[0] = Task.Run(() => _clientSdkHelper.UploadNewPackage(packageId, version: packageVersions[0]));
concurrentTasks[0].Wait();

// push remaining packages over period of 5 seconds
var random = new Random();
for (int i = 1; i < concurrentTasks.Length; i++)
{
var packageVersion = packageVersions[i];
concurrentTasks[i] = Task.Run(() => _clientSdkHelper.UploadNewPackage(packageId, version: packageVersion));
concurrentTasks[i] = Task.Run(async () =>
{
await Task.Delay(random.Next(0, 5000));
await _clientSdkHelper.UploadNewPackage(packageId, version: packageVersion);
});
}
Task.WaitAll(concurrentTasks);

Expand Down