Revert "Fix #2514 - Use optimistic concurrency to prevent multiple Up…

…dateIsLatest calls on same package (#3548)"

This reverts commit 27ea9bc.

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;
@@ -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);
             }
         }
     }

src/NuGetGallery.Core/Entities/EntitiesContext.cs

@@ -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
@@ -69,11 +68,6 @@ public void SetCommandTimeout(int? seconds)
             ObjectContext.CommandTimeout = seconds;
         }
 
-        public DbChangeTracker GetChangeTracker()
-        {
-            return ChangeTracker;
-        }
-
         public Database GetDatabase()
         {
             return Database;

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; }
@@ -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();
     }
 }

src/NuGetGallery/Controllers/ApiController.cs

@@ -473,9 +473,6 @@ await AuditingService.SaveAuditRecordAsync(
                             throw;
                         }
 
-                        // Handle in separate transaction because of concurrency check with retry
-                        await PackageService.UpdateIsLatestAsync(package.PackageRegistration);
-
                         IndexingService.UpdatePackage(package);
 
                         // Write an audit record
@@ -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();
         }
@@ -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();
         }

src/NuGetGallery/Controllers/PackagesController.cs

@@ -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,
@@ -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();
 

src/NuGetGallery/Services/IPackageService.cs

@@ -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>
@@ -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);