Merge pull request #1435 from peppy/fix-threaded-context-issues

Fix write operations potentially using  other-threaded tracked instances

osu.Game/Beatmaps/BeatmapInfo.cs

@@ -6,12 +6,13 @@ using System.ComponentModel.DataAnnotations;
 using System.ComponentModel.DataAnnotations.Schema;
 using System.Linq;
 using Newtonsoft.Json;
+using osu.Game.Database;
 using osu.Game.IO.Serialization;
 using osu.Game.Rulesets;
 
 namespace osu.Game.Beatmaps
 {
-    public class BeatmapInfo : IEquatable<BeatmapInfo>, IJsonSerializable
+    public class BeatmapInfo : IEquatable<BeatmapInfo>, IJsonSerializable, IHasPrimaryKey
     {
         [DatabaseGenerated(DatabaseGeneratedOption.Identity)]
         public int ID { get; set; }

osu.Game/Beatmaps/BeatmapSetInfo.cs

@@ -4,10 +4,11 @@
 using System.Collections.Generic;
 using System.ComponentModel.DataAnnotations.Schema;
 using System.Linq;
+using osu.Game.Database;
 
 namespace osu.Game.Beatmaps
 {
-    public class BeatmapSetInfo
+    public class BeatmapSetInfo : IHasPrimaryKey
     {
         [DatabaseGenerated(DatabaseGeneratedOption.Identity)]
         public int ID { get; set; }

osu.Game/Beatmaps/BeatmapStore.cs

@@ -48,10 +48,10 @@ namespace osu.Game.Beatmaps
         {
             var context = GetContext();
 
-            if (beatmapSet.DeletePending) return false;
+            Refresh(ref beatmapSet, BeatmapSets);
 
+            if (beatmapSet.DeletePending) return false;
             beatmapSet.DeletePending = true;
-            context.Update(beatmapSet);
             context.SaveChanges();
 
             BeatmapSetRemoved?.Invoke(beatmapSet);
@@ -67,10 +67,10 @@ namespace osu.Game.Beatmaps
         {
             var context = GetContext();
 
-            if (!beatmapSet.DeletePending) return false;
+            Refresh(ref beatmapSet, BeatmapSets);
 
+            if (!beatmapSet.DeletePending) return false;
             beatmapSet.DeletePending = false;
-            context.Update(beatmapSet);
             context.SaveChanges();
 
             BeatmapSetAdded?.Invoke(beatmapSet);
@@ -86,10 +86,10 @@ namespace osu.Game.Beatmaps
         {
             var context = GetContext();
 
-            if (beatmap.Hidden) return false;
+            Refresh(ref beatmap, Beatmaps);
 
+            if (beatmap.Hidden) return false;
             beatmap.Hidden = true;
-            context.Update(beatmap);
             context.SaveChanges();
 
             BeatmapHidden?.Invoke(beatmap);
@@ -105,10 +105,10 @@ namespace osu.Game.Beatmaps
         {
             var context = GetContext();
 
-            if (!beatmap.Hidden) return false;
+            Refresh(ref beatmap, Beatmaps);
 
+            if (!beatmap.Hidden) return false;
             beatmap.Hidden = false;
-            context.Update(beatmap);
             context.SaveChanges();
 
             BeatmapRestored?.Invoke(beatmap);

osu.Game/Database/DatabaseBackedStore.cs

@@ -2,7 +2,10 @@
 // Licensed under the MIT Licence - https://raw.githubusercontent.com/ppy/osu/master/LICENCE
 
 using System;
+using System.Collections.Generic;
+using System.Linq;
 using System.Threading;
+using Microsoft.EntityFrameworkCore;
 using osu.Framework.Platform;
 
 namespace osu.Game.Database
@@ -18,6 +21,22 @@ namespace osu.Game.Database
 
         private readonly ThreadLocal<OsuDbContext> queryContext;
 
+        /// <summary>
+        /// Refresh an instance potentially from a different thread with a local context-tracked instance.
+        /// </summary>
+        /// <param name="obj">The object to use as a reference when negotiating a local instance.</param>
+        /// <param name="lookupSource">An optional lookup source which will be used to query and populate a freshly retrieved replacement. If not provided, the refreshed object will still be returned but will not have any includes.</param>
+        /// <typeparam name="T">A valid EF-stored type.</typeparam>
+        protected virtual void Refresh<T>(ref T obj, IEnumerable<T> lookupSource = null) where T : class, IHasPrimaryKey
+        {
+            var context = GetContext();
+
+            if (context.Entry(obj).State != EntityState.Detached) return;
+
+            var id = obj.ID;
+            obj = lookupSource?.SingleOrDefault(t => t.ID == id) ?? context.Find<T>(id);
+        }
+
         /// <summary>
         /// Retrieve a shared context for performing lookups (or write operations on the update thread, for now).
         /// </summary>

osu.Game/Database/IHasPrimaryKey.cs

@@ -0,0 +1,10 @@
+// Copyright (c) 2007-2017 ppy Pty Ltd <contact@ppy.sh>.
+// Licensed under the MIT Licence - https://raw.githubusercontent.com/ppy/osu/master/LICENCE
+
+namespace osu.Game.Database
+{
+    public interface IHasPrimaryKey
+    {
+        int ID { get; set; }
+    }
+}

osu.Game/Input/Bindings/DatabasedKeyBinding.cs

@@ -3,11 +3,12 @@
 
 using System.ComponentModel.DataAnnotations.Schema;
 using osu.Framework.Input.Bindings;
+using osu.Game.Database;
 
 namespace osu.Game.Input.Bindings
 {
     [Table("KeyBinding")]
-    public class DatabasedKeyBinding : KeyBinding
+    public class DatabasedKeyBinding : KeyBinding, IHasPrimaryKey
     {
         [DatabaseGenerated(DatabaseGeneratedOption.Identity)]
         public int ID { get; set; }

osu.Game/Input/KeyBindingStore.cs

@@ -60,7 +60,7 @@ namespace osu.Game.Input
         }
 
         /// <summary>
-        /// Retrieve <see cref="KeyBinding"/>s for a specified ruleset/variant content.
+        /// Retrieve <see cref="DatabasedKeyBinding"/>s for a specified ruleset/variant content.
         /// </summary>
         /// <param name="rulesetId">The ruleset's internal ID.</param>
         /// <param name="variant">An optional variant.</param>
@@ -70,8 +70,14 @@ namespace osu.Game.Input
 
         public void Update(KeyBinding keyBinding)
         {
+            var dbKeyBinding = (DatabasedKeyBinding)keyBinding;
+
             var context = GetContext();
-            context.Update(keyBinding);
+
+            Refresh(ref dbKeyBinding);
+
+            dbKeyBinding.KeyCombination = keyBinding.KeyCombination;
+
             context.SaveChanges();
 
             KeyBindingChanged?.Invoke();

osu.Game/Screens/Select/BeatmapCarousel.cs

@@ -119,7 +119,7 @@ namespace osu.Game.Screens.Select
         internal void UpdateBeatmap(BeatmapInfo beatmap)
         {
             // todo: this method should not run more than once for the same BeatmapSetInfo.
-            var set = manager.Refresh(beatmap.BeatmapSet);
+            var set = manager.QueryBeatmapSet(s => s.ID == beatmap.BeatmapSetInfoID);
 
             // todo: this method should be smarter as to not recreate panels that haven't changed, etc.
             var group = groups.Find(b => b.BeatmapSet.ID == set.ID);

osu.Game/osu.Game.csproj

@@ -282,6 +282,7 @@
     <Compile Include="Beatmaps\Drawables\BeatmapSetCover.cs" />
     <Compile Include="Beatmaps\Drawables\BeatmapSetHeader.cs" />
     <Compile Include="Database\DatabaseContextFactory.cs" />
+    <Compile Include="Database\IHasPrimaryKey.cs" />
     <Compile Include="Migrations\20171019041408_InitialCreate.cs" />
     <Compile Include="Migrations\20171019041408_InitialCreate.Designer.cs">
       <DependentUpon>20171019041408_InitialCreate.cs</DependentUpon>