Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
Revert UpdateIsLatest optimistic concurrency changes (#3707)
* Revert "UpdateIsLatest concurrent unlist fix (#3695)"

This reverts commit 551fd86.

* Revert "Fix concurrent push test by disabling search hijacking on feed (#3641)"

This reverts commit 00fb3fb.

* Revert "IsLatest Fix: wrong connection string passed to retry context (#3632)"

This reverts commit 1b6b5b0.

* Revert "Fix #2514 - Use optimistic concurrency to prevent multiple UpdateIsLatest calls on same package (#3548)"

This reverts commit 27ea9bc.
  • Loading branch information
chenriksson committed Mar 27, 2017
1 parent 551fd86 commit 9ccd227
Show file tree
Hide file tree
Showing 17 changed files with 294 additions and 703 deletions.
33 changes: 5 additions & 28 deletions src/NuGetGallery.Core/Entities/EntitiesConfiguration.cs
@@ -1,7 +1,6 @@
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System;
using System.Data.Entity;
using System.Data.Entity.Infrastructure;
using System.Data.Entity.SqlServer;
Expand All @@ -16,41 +15,19 @@ public EntitiesConfiguration()
{
// Configure Connection Resiliency / Retry Logic
// See https://msdn.microsoft.com/en-us/data/dn456835.aspx and msdn.microsoft.com/en-us/data/dn307226
SetExecutionStrategy("System.Data.SqlClient", () => UseRetriableExecutionStrategy
? new SqlAzureExecutionStrategy() : (IDbExecutionStrategy)new DefaultExecutionStrategy());
SetExecutionStrategy("System.Data.SqlClient", () => SuspendExecutionStrategy
? (IDbExecutionStrategy)new DefaultExecutionStrategy() : new SqlAzureExecutionStrategy());
}

private static bool UseRetriableExecutionStrategy
public static bool SuspendExecutionStrategy
{
get
{
return (bool?)CallContext.LogicalGetData(nameof(UseRetriableExecutionStrategy)) ?? true;
return (bool?)CallContext.LogicalGetData("SuspendExecutionStrategy") ?? false;
}
set
{
CallContext.LogicalSetData(nameof(UseRetriableExecutionStrategy), value);
}
}

public static IDisposable SuspendRetriableExecutionStrategy()
{
return new RetriableExecutionStrategySuspension();
}

private class RetriableExecutionStrategySuspension : IDisposable
{
private readonly bool _originalValue;

internal RetriableExecutionStrategySuspension()
{
_originalValue = UseRetriableExecutionStrategy;

UseRetriableExecutionStrategy = false;
}

public void Dispose()
{
UseRetriableExecutionStrategy = _originalValue;
CallContext.LogicalSetData("SuspendExecutionStrategy", value);
}
}
}
Expand Down
6 changes: 0 additions & 6 deletions src/NuGetGallery.Core/Entities/EntitiesContext.cs
Expand Up @@ -2,7 +2,6 @@
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System.Data.Entity;
using System.Data.Entity.Infrastructure;
using System.Threading.Tasks;

namespace NuGetGallery
Expand Down Expand Up @@ -69,11 +68,6 @@ public void SetCommandTimeout(int? seconds)
ObjectContext.CommandTimeout = seconds;
}

public DbChangeTracker GetChangeTracker()
{
return ChangeTracker;
}

public Database GetDatabase()
{
return Database;
Expand Down
5 changes: 0 additions & 5 deletions src/NuGetGallery.Core/Entities/IEntitiesContext.cs
@@ -1,15 +1,12 @@
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System;
using System.Data.Entity;
using System.Data.Entity.Infrastructure;
using System.Threading.Tasks;

namespace NuGetGallery
{
public interface IEntitiesContext
: IDisposable
{
IDbSet<CuratedFeed> CuratedFeeds { get; set; }
IDbSet<CuratedPackage> CuratedPackages { get; set; }
Expand All @@ -23,8 +20,6 @@ public interface IEntitiesContext
IDbSet<T> Set<T>() where T : class;
void DeleteOnCommit<T>(T entity) where T : class;
void SetCommandTimeout(int? seconds);

DbChangeTracker GetChangeTracker();
Database GetDatabase();
}
}
14 changes: 0 additions & 14 deletions src/NuGetGallery/Controllers/ApiController.cs
Expand Up @@ -473,9 +473,6 @@ private async Task<ActionResult> CreatePackageInternal()
throw;
}

// Handle in separate transaction because of concurrency check with retry
await PackageService.UpdateIsLatestAsync(package.PackageRegistration);

IndexingService.UpdatePackage(package);

// Write an audit record
Expand Down Expand Up @@ -555,13 +552,6 @@ public virtual async Task<ActionResult> DeletePackage(string id, string version)
}

await PackageService.MarkPackageUnlistedAsync(package);

// Handle in separate transaction because of concurrency check with retry. Due to using
// separate transactions, we must always call UpdateIsLatest on delete/unlist. This is
// because a concurrent thread could be marking the package as latest before this thread
// is able to commit the delete /unlist.
await PackageService.UpdateIsLatestAsync(package.PackageRegistration);

IndexingService.UpdatePackage(package);
return new EmptyResult();
}
Expand Down Expand Up @@ -595,10 +585,6 @@ public virtual async Task<ActionResult> PublishPackage(string id, string version
}

await PackageService.MarkPackageListedAsync(package);

// handle in separate transaction because of concurrency check with retry
await PackageService.UpdateIsLatestAsync(package.PackageRegistration);

IndexingService.UpdatePackage(package);
return new EmptyResult();
}
Expand Down
12 changes: 0 additions & 12 deletions src/NuGetGallery/Controllers/PackagesController.cs
Expand Up @@ -999,20 +999,11 @@ internal virtual async Task<ActionResult> Edit(string id, string version, bool?
{
action = "unlisted";
await _packageService.MarkPackageUnlistedAsync(package);

// Handle in separate transaction because of concurrency check with retry. Due to using
// separate transactions, we must always call UpdateIsLatest on delete/unlist. This is
// because a concurrent thread could be marking the package as latest before this thread
// is able to commit the delete /unlist.
await _packageService.UpdateIsLatestAsync(package.PackageRegistration);
}
else
{
action = "listed";
await _packageService.MarkPackageListedAsync(package);

// handle in separate transaction because of concurrency check with retry
await _packageService.UpdateIsLatestAsync(package.PackageRegistration);
}
TempData["Message"] = String.Format(
CultureInfo.CurrentCulture,
Expand Down Expand Up @@ -1204,9 +1195,6 @@ public virtual async Task<ActionResult> VerifyPackage(VerifyPackageRequest formD
throw;
}

// handle in separate transaction because of concurrency check with retry
await _packageService.UpdateIsLatestAsync(package.PackageRegistration);

// tell Lucene to update index for the new package
_indexingService.UpdateIndex();

Expand Down
51 changes: 1 addition & 50 deletions src/NuGetGallery/Services/IPackageService.cs
Expand Up @@ -16,23 +16,13 @@ public interface IPackageService
IEnumerable<PackageRegistration> FindPackageRegistrationsByOwner(User user);
IEnumerable<Package> FindDependentPackages(Package package);

/// <summary>
/// Updates IsLatest/IsLatestStable flags after a package create, update or delete operation.
///
/// Due to the manual optimistic concurrency check added to the underlying implementation,
/// IsLatest/IsLatestStable changes will be applied in memory and should not be committed with EF.
/// Callers should ensure that all other commits are complete before calling UpdateIsLatestAsync.
/// </summary>
/// <param name="packageRegistration"></param>
/// <returns></returns>
Task UpdateIsLatestAsync(PackageRegistration packageRegistration);
Task UpdateIsLatestAsync(PackageRegistration packageRegistration, bool commitChanges = true);

/// <summary>
/// Populate the related database tables to create the specified package for the specified user.
/// </summary>
/// <remarks>
/// This method doesn't upload the package binary to the blob storage. The caller must do it after this call.
/// This method doesn't update IsLatest/IsLatestStable flags. The caller must do it after this call.
/// </remarks>
/// <param name="nugetPackage">The package to be created.</param>
/// <param name="packageStreamMetadata">The package stream's metadata.</param>
Expand All @@ -43,49 +33,10 @@ public interface IPackageService

Package EnrichPackageFromNuGetPackage(Package package, PackageArchiveReader packageArchive, PackageMetadata packageMetadata, PackageStreamMetadata packageStreamMetadata, User user);

/// <summary>
/// Publishes a package by listing it.
/// </summary>
/// <remarks>
/// This method doesn't update IsLatest/IsLatestStable flags. The caller must do it after this call.
/// </remarks>
/// <param name="id">ID for the package to publish</param>
/// <param name="version">Package version</param>
/// <param name="commitChanges">Whether to commit changes to the database.</param>
/// <returns></returns>
Task PublishPackageAsync(string id, string version, bool commitChanges = true);

/// <summary>
/// Publishes a package by listing it.
/// </summary>
/// <remarks>
/// This method doesn't update IsLatest/IsLatestStable flags. The caller must do it after this call.
/// </remarks>
/// <param name="package">The package to publish</param>
/// <param name="commitChanges">Whether to commit changes to the database.</param>
/// <returns></returns>
Task PublishPackageAsync(Package package, bool commitChanges = true);

/// <summary>
/// Mark a package as unlisted.
/// </summary>
/// <remarks>
/// This method doesn't update IsLatest/IsLatestStable flags. The caller must do it after this call.
/// </remarks>
/// <param name="package">The package to unlist</param>
/// <param name="commitChanges">Whether to commit changes to the database.</param>
/// <returns></returns>
Task MarkPackageUnlistedAsync(Package package, bool commitChanges = true);

/// <summary>
/// Mark a package as listed.
/// </summary>
/// <remarks>
/// This method doesn't update IsLatest/IsLatestStable flags. The caller must do it after this call.
/// </remarks>
/// <param name="package">The package to list.</param>
/// <param name="commitChanges">Whether to commit changes to the database.</param>
/// <returns></returns>
Task MarkPackageListedAsync(Package package, bool commitChanges = true);

Task<PackageOwnerRequest> CreatePackageOwnerRequestAsync(PackageRegistration package, User currentOwner, User newOwner);
Expand Down

0 comments on commit 9ccd227

Please sign in to comment.