Improve transaction handling flexibility

osu.Game/Database/ArchiveModelManager.cs

@@ -56,13 +56,42 @@ namespace osu.Game.Database
         // ReSharper disable once NotAccessedField.Local (we should keep a reference to this so it is not finalised)
         private ArchiveImportIPCChannel ipc;
 
+        private readonly List<Action> cachedEvents = new List<Action>();
+
+        private bool delayingEvents;
+
+        private void cacheEvents()
+        {
+            delayingEvents = true;
+        }
+
+        private void flushEvents(bool perform)
+        {
+            if (perform)
+            {
+                foreach (var a in cachedEvents)
+                    a.Invoke();
+            }
+
+            cachedEvents.Clear();
+            delayingEvents = false;
+        }
+
+        private void handleEvent(Action a)
+        {
+            if (delayingEvents)
+                cachedEvents.Add(a);
+            else
+                a.Invoke();
+        }
+
         protected ArchiveModelManager(Storage storage, IDatabaseContextFactory contextFactory, MutableDatabaseBackedStore<TModel> modelStore, IIpcHost importHost = null)
         {
             ContextFactory = contextFactory;
 
             ModelStore = modelStore;
-            ModelStore.ItemAdded += s => ItemAdded?.Invoke(s);
+            ModelStore.ItemAdded += s => handleEvent(() => ItemAdded?.Invoke(s));
-            ModelStore.ItemRemoved += s => ItemRemoved?.Invoke(s);
+            ModelStore.ItemRemoved += s => handleEvent(() => ItemRemoved?.Invoke(s));
 
             Files = new FileStore(contextFactory, storage);
 
@@ -138,24 +167,37 @@ namespace osu.Game.Database
         /// <param name="archive">The archive to be imported.</param>
         public TModel Import(ArchiveReader archive)
         {
-            using (ContextFactory.GetForWrite()) // used to share a context for full import. keep in mind this will block all writes.
+            TModel item = null;
+            cacheEvents();
+
+            try
             {
-                // create a new model (don't yet add to database)
+                using (var write = ContextFactory.GetForWrite()) // used to share a context for full import. keep in mind this will block all writes.
-                var item = CreateModel(archive);
+                {
+                    if (!write.IsTransactionLeader) throw new InvalidOperationException($"Ensure there is no parent transaction so errors can correctly be handled by {this}");
 
-                var existing = CheckForExisting(item);
+                    // create a new model (don't yet add to database)
+                    item = CreateModel(archive);
 
-                if (existing != null) return existing;
+                    var existing = CheckForExisting(item);
 
-                item.Files = createFileInfos(archive, Files);
+                    if (existing != null) return existing;
 
-                Populate(item, archive);
+                    item.Files = createFileInfos(archive, Files);
 
-                // import to store
+                    Populate(item, archive);
-                ModelStore.Add(item);
 
-                return item;
+                    // import to store
+                    ModelStore.Add(item);
+                }
             }
+            catch
+            {
+                item = null;
+            }
+
+            flushEvents(item != null);
+            return item;
         }
 
         /// <summary>

osu.Game/Database/DatabaseContextFactory.cs

@@ -1,7 +1,9 @@
 // Copyright (c) 2007-2018 ppy Pty Ltd <contact@ppy.sh>.
 // Licensed under the MIT Licence - https://raw.githubusercontent.com/ppy/osu/master/LICENCE
 
+using System.Linq;
 using System.Threading;
+using Microsoft.EntityFrameworkCore.Storage;
 using osu.Framework.Platform;
 
 namespace osu.Game.Database
@@ -17,8 +19,12 @@ namespace osu.Game.Database
         private readonly object writeLock = new object();
 
         private bool currentWriteDidWrite;
+        private bool currentWriteDidError;
+
         private int currentWriteUsages;
 
+        private IDbContextTransaction currentWriteTransaction;
+
         public DatabaseContextFactory(GameHost host)
         {
             this.host = host;
@@ -40,9 +46,12 @@ namespace osu.Game.Database
         {
             Monitor.Enter(writeLock);
 
+            if (currentWriteTransaction == null)
+                currentWriteTransaction = threadContexts.Value.Database.BeginTransaction();
+
             Interlocked.Increment(ref currentWriteUsages);
 
-            return new DatabaseWriteUsage(threadContexts.Value, usageCompleted);
+            return new DatabaseWriteUsage(threadContexts.Value, usageCompleted) { IsTransactionLeader = currentWriteUsages == 1 };
         }
 
         private void usageCompleted(DatabaseWriteUsage usage)
@@ -52,16 +61,24 @@ namespace osu.Game.Database
             try
             {
                 currentWriteDidWrite |= usage.PerformedWrite;
+                currentWriteDidError |= usage.Errors.Any();
 
                 if (usages > 0) return;
 
+                if (currentWriteDidError)
+                    currentWriteTransaction.Rollback();
+                else
+                    currentWriteTransaction.Commit();
+
+                currentWriteTransaction = null;
+                currentWriteDidWrite = false;
+                currentWriteDidError = false;
+
                 if (currentWriteDidWrite)
                 {
                     // explicitly dispose to ensure any outstanding flushes happen as soon as possible (and underlying resources are purged).
                     usage.Context.Dispose();
 
-                    currentWriteDidWrite = false;
-
                     // once all writes are complete, we want to refresh thread-specific contexts to make sure they don't have stale local caches.
                     recycleThreadContexts();
                 }

osu.Game/Database/DatabaseWriteUsage.cs

@@ -2,26 +2,31 @@
 // Licensed under the MIT Licence - https://raw.githubusercontent.com/ppy/osu/master/LICENCE
 
 using System;
-using Microsoft.EntityFrameworkCore.Storage;
+using System.Collections.Generic;
 
 namespace osu.Game.Database
 {
     public class DatabaseWriteUsage : IDisposable
     {
         public readonly OsuDbContext Context;
-        private readonly IDbContextTransaction transaction;
         private readonly Action<DatabaseWriteUsage> usageCompleted;
 
         public DatabaseWriteUsage(OsuDbContext context, Action<DatabaseWriteUsage> onCompleted)
         {
             Context = context;
-            transaction = Context.BeginTransaction();
             usageCompleted = onCompleted;
         }
 
         public bool PerformedWrite { get; private set; }
 
         private bool isDisposed;
+        public List<Exception> Errors = new List<Exception>();
+
+        /// <summary>
+        /// Whether this write usage will commit a transaction on completion.
+        /// If false, there is a parent usage responsible for transaction commit.
+        /// </summary>
+        public bool IsTransactionLeader = false;
 
         protected void Dispose(bool disposing)
         {
@@ -30,12 +35,11 @@ namespace osu.Game.Database
 
             try
             {
-                PerformedWrite |= Context.SaveChanges(transaction) > 0;
+                PerformedWrite |= Context.SaveChanges() > 0;
             }
             catch (Exception e)
             {
-                transaction?.Rollback();
+                Errors.Add(e);
-                throw;
             }
             finally
             {

osu.Game/Database/OsuDbContext.cs

@@ -3,7 +3,6 @@
 
 using System;
 using Microsoft.EntityFrameworkCore;
-using Microsoft.EntityFrameworkCore.Storage;
 using Microsoft.EntityFrameworkCore.Diagnostics;
 using Microsoft.Extensions.Logging;
 using osu.Framework.Logging;
@@ -104,18 +103,6 @@ namespace osu.Game.Database
             modelBuilder.Entity<BeatmapInfo>().HasOne(b => b.BaseDifficulty);
         }
 
-        public IDbContextTransaction BeginTransaction()
-        {
-            return Database.BeginTransaction();
-        }
-
-        public int SaveChanges(IDbContextTransaction transaction = null)
-        {
-            var ret = base.SaveChanges();
-            if (ret > 0) transaction?.Commit();
-            return ret;
-        }
-
         private class OsuDbLoggerFactory : ILoggerFactory
         {
             #region Disposal