Upgrade all dependencies #11612

hez2010 · 2021-01-27T19:18:22Z

An attempt of upgrading all dependencies to latest, to see whether things are going to work or not.

bdach · 2021-01-27T19:32:32Z

Several issues, even if it passes CI (which it won't, due to a code style failure - there's a leftover using statement in OsuDbContext).

Warnings appearing on game launch:

Compiling a query which loads related collections for more than one collection navigation either via 'Include' or through projection but no 'QuerySplittingBehavior' has been configured. By default Entity Framework will use 'QuerySplittingBehavior.SingleQuery' which can potentially result in slow query performance. See https://go.microsoft.com/fwlink/?linkid=2134277 for more information. To identify the query that's triggering this warning call 'ConfigureWarnings(w => w.Throw(RelationalEventId.MultipleCollectionIncludeWarning))'

You've even attached those in discord and them promptly ignored them. That can't land in a release. We'll get swamped with issue reports.

Database imports intermittently don't work.

2021-01-27 19:26:17 [error]: [b6f3b] Database import or population failed and has been rolled back.
2021-01-27 19:26:17 [error]: System.InvalidOperationException: The instance of entity type 'FileInfo' cannot be tracked because another instance with the same key value for {'ID'} is already being tracked. When attaching existing entities, ensure that only one entity instance with a given key value is attached. Consider using 'DbContextOptionsBuilder.EnableSensitiveDataLogging' to see the conflicting key values.
2021-01-27 19:26:17 [error]: at Microsoft.EntityFrameworkCore.ChangeTracking.Internal.IdentityMap`1.ThrowIdentityConflict(InternalEntityEntry entry)
2021-01-27 19:26:17 [error]: at Microsoft.EntityFrameworkCore.ChangeTracking.Internal.IdentityMap`1.Add(TKey key, InternalEntityEntry entry, Boolean updateDuplicate)
2021-01-27 19:26:17 [error]: at Microsoft.EntityFrameworkCore.ChangeTracking.Internal.IdentityMap`1.Add(TKey key, InternalEntityEntry entry)

SignalR was also held back for a reason (on iOS at least) and this PR bumps it again despite warnings that there was a reason not to do so. That won't fail CI as we don't run CI on iOS.

If it were just an issue of bumping the versions we'd have done so by now. Are you going to be trying to fix these issues at all?

bdach

as above

hez2010 · 2021-01-27T19:40:37Z

I'm trying to fix them in next several days.
I think it's ok to close this PR, and if I could get things work, I will send another PR.

peppy · 2021-01-27T20:52:45Z

For the SignalR side, I was looking at bumping/testing it yesterday and was blocked by the dependency chain. I'll take a look at that part on this branch, as it is likely resolved (see #osu-lazer backlog for the troubles I had with dependency resolution).

The database import failures are probably the same thing I was seeing last time I attempted to upgrade to the newer ef-core. I believe the cause is that we are doing threading wrong. Again, if there's a simple fix for this then we can definitely go with it, but if it's a larger conceptual change/refactor it may still be worth waiting on the realm path forward.

peppy · 2021-01-28T04:55:36Z

Have tested against iOS and things look... as good as they can be. Ran into an issue with SignalR AOT, but that is going to be an issue regardless (hoping for a reply at MessagePack-CSharp/MessagePack-CSharp#780 (comment)). I will take a look into the import issues next.

peppy · 2021-01-28T07:08:40Z

@hez2010 are you already looking into the tracking entity issues? if so i'll stop doing so until you've come to a conclusion and focus elsewhere.

hez2010 · 2021-01-28T07:35:05Z

@hez2010 are you already looking into the tracking entity issues? if so i'll stop doing so until you've come to a conclusion and focus elsewhere.

Found some clues:
In ArchiveModelManager(line 363) -> MutableDatabaseBackedStore<T = BeatmapSetInfo> (line 48): context.Attach(item)
In some cases (item: BeatmapSetInfo).(Files: List<BeatmapSetFileInfo>).(FileInfo: FileInfo) is already being loaded and tracked which causes the tracking problem.

I think a possible solution is to split FileInfo addition and beatmap addition into two stages: adding FileInfos (or querying FileInfoID) first and calling SaveChanges, and then processing beatmap with quried FileInfoID in another transaction. If errors occurred during processing beatmaps, I think there's no need to roll back added FileInfos because they may be used in the future.

peppy · 2021-01-28T08:04:23Z

@hez2010 i also looked into the same path, but i also:

moved the file addition inside the main transaction (just for testing purposes; it's not as easy to do as you may have found out due to the await causing potential threading issue)
removed the "refetech" in FileStore.Reference / Dereference

At this point, the FileInfos which are returned by FileStore are 100% the tracked entities, which are then being added to the beatmap's files, and finally Attached to the database (I tried changing the Attach to Add just in case, and that saw no difference). To my knowledge, this seems to be a correct flow, yet the error still occurs so I was a bit lost.

If you are able to make it work with your strategy of a double pass, then that should be sufficient for now, given that we are hopefully moving away from EF in the future so this is all temporary.

I'll leave it in your hands for now; let me know if you give up and I will try again.

hez2010 · 2021-01-28T08:49:42Z

~~@peppy I managed to solve the tracking issue by setting tracked entries to null and use their ID instead, and then restore entries after imported. Although this approach may be a little tricky.~~

hez2010 · 2021-01-28T09:20:57Z

I used dotnet format to format code to make CI pass, which introduced no actual code changes.
Only 2 files have actual code changes in this PR:

osu.Game/Database/ArchiveModelManager.cs
osu.Game/Database/OsuDbContext.cs

bdach · 2021-01-28T09:51:27Z

...this has 300 files changed now and a diffstat in thousands of lines. the PR is now borderline unreviewable with code changes intermingled with formatting changes and dependency bumps. that is not how things should ever be done.

This reverts commit 7a9ec4c.

peppy · 2021-01-28T10:21:17Z

I've reverted most of the changes. Please only commit further database fixes to this branch or we will be forces to start a separate branch from scratch.

hez2010 · 2021-01-28T19:55:51Z

@peppy All tracking issues of migrations and imports have been resolved via splitting one transaction into multiple transactions.

Now the last issue remaining:
I don't know why there're conflicts between RulesetInfo. I guess 0 may be considered as a valid key when its type is int?, which causes conflict keys between RulesetInfo:

In test code (such as osu.Game.Tests/TestSceneMatchSongSelect):

new BeatmapInfo
{
    // ...
    Ruleset = new OsuRuleset().RulesetInfo // boom
},

osu.Game/Beatmaps/BeatmapStore.cs

osu.Game/Database/ArchiveModelManager.cs

bdach · 2021-01-28T20:18:17Z

osu.Game/Scoring/ScoreInfo.cs

        [JsonIgnore]
        [Column("Mods")]
-        public string ModsJson


what was wrong with this property?

Since Mods is not mapped and ModsJson is mapped as Mods column, only changes to ModsJson can make efcore 5 detect changes in Mods. So here we should apply changes to ModJson to make sure changes are being tracked by efcore, and use Mods for json deserialization. Otherwise some tests will also fail.

bdach · 2021-01-28T20:19:17Z

not sure how to review this change without going myself and reverse-engineering it all, without any actual reasoning provided for any of these changes.

hez2010 · 2021-01-29T14:43:59Z

@bdach Most changes you previously reviewed have been reverted.
@peppy I think the tracking issue is resolved and all tests should pass now.
The tracking issue was caused for both modified FileInfo (or RulesetInfo) entries and original FileInfo (or RulesetInfo) entries are existing in the entity item, so that FileInfo (or RulesetInfo) entries with same ID are conflicting with each other.

My solution is always looking up latest entries and replacing the old same ID entries before importing.

bdach · 2021-01-29T20:06:36Z

osu.Game/Beatmaps/BeatmapManager.cs

+            for (int i = beatmapSet.Files.Count - 1; i >= 0; i--)
+            {
+                var file = beatmapSet.Files[i];
+                var id = file?.FileInfo?.ID;
+
+                if (id != null && id != 0)
+                {
+                    if (latestFileInfos.ContainsKey(id.Value))
+                    {
+                        file.FileInfo = latestFileInfos[id.Value];
+                    }
+                    else
+                    {
+                        latestFileInfos[id.Value] = file.FileInfo;
+                    }
+                }
+            }


how does this work? this silently assumes that the last beatmap set was fetched from EF last, and therefore has the freshest entity?

not sure i like this at all. it's very precarious. would probably rather just do a full refetch of everything, definitely feels much safer.

New modified FileInfos will be appended to Files, and with changes of Reference and Dereference, old entries in the list will be updated by ref, so here we only need to keep the last modified one.

bdach · 2021-01-29T20:53:36Z

osu.Game/IO/FileStore.cs


+        public void Reference(ref FileInfo file)


not sure on doing the refetch as part of the {Reference,Dereference} operations. looks super awkward and clunky (especially in usage code). would just do that explicitly via Find.

It won't going to work. Any modification to the one which loaded from database via Find doesn't affect the originally exist one, after calling Update in FileStore the modified one will be tracked, so the later call to Add in ArchiveModelManager will lead to multiple FileInfo with same ID but different ReferenceCount coexist in ChangeTracker and cause tracking conflicts.

The cheapest solution is to use the modified one to replace the original one after Update, here I use ref to achieve this.

I also had a similar implementation to this in my attempt to fix this. I don't think it's too bad.

bdach · 2021-01-29T21:03:49Z

osu.Game/Database/ArchiveModelManager.cs

+                        write.Context.ChangeTracker.Entries()
+                             .Where(e => e.State == EntityState.Unchanged)
+                             .ToList()
+                             .ForEach(e => e.State = EntityState.Detached);


this "detach everything and try saving" actually seems like a more promising way to do all of this. if we use that approach everywhere, will it work?

just feels like a simpler way to deal.

Unfortunately it only solves the problem when entities already in ChangeTracker before calling Add, but entities with same id in item will still cause tracking conflict issues.

peppy · 2021-02-01T08:00:24Z

osu.Game/Beatmaps/BeatmapManager.cs

@@ -366,8 +411,7 @@ public IEnumerable<BeatmapSetInfo> GetAllUsableBeatmapSetsEnumerable(IncludedDet

            // AsEnumerable used here to avoid applying the WHERE in sql. When done so, ef core 2.x uses an incorrect ORDER BY
            // clause which causes queries to take 5-10x longer.
-            // TODO: remove if upgrading to EF core 3.x.
-            return queryable.AsEnumerable().Where(s => !s.DeletePending && (includeProtected || !s.Protected));
+            return queryable.Where(s => !s.DeletePending && (includeProtected || !s.Protected));


The remaining comment above can also be removed if this is deemed resolved.

peppy · 2021-02-01T08:02:11Z

osu.Game/IO/FileStore.cs


+        public void Reference(ref FileInfo file)


I also had a similar implementation to this in my attempt to fix this. I don't think it's too bad.

peppy · 2021-02-01T08:06:48Z

osu.Game/Scoring/ScoreManager.cs

@@ -88,6 +89,42 @@ protected override bool CheckLocalAvailability(ScoreInfo model, IQueryable<Score
            => base.CheckLocalAvailability(model, items)
               || (model.OnlineScoreID != null && items.Any(i => i.OnlineScoreID == model.OnlineScoreID));

+        protected override void PreImport(ScoreInfo model)
+        {
+            var latestFileInfos = new Dictionary<int, FileInfo>();


what is this whole block doing?

Some tracking issues were caused by FileInfo in .Files with same ID but different ReferenceCount, so it enumerates the list by desc and use latest entries to replace old entries (since the newly modified one must be ranked behind the previous one in List<T>). Although I don't know why there're FileInfo entries with same ID in the list.

Same for RulesetInfo.

I'd personally be more fine with this sort of trick if we could ask EF for the newest entry, instead of essentially guessing at random that the entities at the end of the list are the newest entry.

The modifications haven't been committed into database yet so there won't be latest entries from EF.

very confused as to what committing to DB has to do with EF entry tracking. this is a DbContext level issue, correct? can't DbContext.ChangeTracker.Entries() be used or something?

Both modified and unmodified entries are in item which are causing tracking conflicts while adding it to ChangeTracker, and before calling Add(item) there're no entries in ChangeTracker.

hez2010 · 2021-02-02T16:21:24Z

@peppy A more elegant way without these tricks is to use lazy loading mode in EF, which can avoid most all above problems. But this would require large code changes.

peppy · 2021-02-09T08:00:18Z

I do believe this PR is going to be stuck in a permanent stalemate with the current state of changes. While what you've added here may work correctly, there's just too many individual hacks to instill any degree of confidence. There's a high chance that something else is going to break as a result of these changes and either go unnoticed or cause new bugs to arise.

hez2010 · 2021-02-09T08:24:57Z

I think the best solution is using ef lazy loading mode to rewrite storage classes which can solve the tracking issues without needs of current tricks, but it would require large code changes. However, since osu is moving away from ef, I don't see the necessity of making efforts to write something that will soon be replaced.

hez2010 · 2021-03-01T11:39:11Z

@peppy
Since efcore 6 resolved the compatibility issue with AOT compiler via dotnet/efcore#24211, I'll try to reimplement the storage layer to make it work and then upgrade to .NET 6 at .NET 6 time phase if you are not planning to migrate to Realm until that time.

What do you think?

peppy · 2021-03-01T12:31:33Z

That sounds fine, but only if you are okay with the work you do potentially not being used (we can't guarantee it will be merged, depending on complexity). We are still intent on switching to Realm but waiting on an upstream threading crash that needs to be resolved before we can deploy it.

peppy · 2021-03-15T04:34:36Z

@hez2010 thanks for your work on this. I'm going to close this in favour of #12003, which reduces the number of workarounds required to get this working. Probably wouldn't have happened without this PR pushing things in this direction, so hopefully it doesn't feel like this being closed makes it in vain.

Dependencies upgrade

d7f6d81

pull-request-size bot added the size/M label Jan 27, 2021

bdach requested changes Jan 27, 2021

View reviewed changes

peppy added area:tooling realm deals with local realm database labels Jan 28, 2021

Resolve ef warnings on startup

ae9f380

peppy mentioned this pull request Jan 28, 2021

Fallback to using json for signalr communication if JIT is unavailable #11615

Merged

Fix tracking issues

1ab72e2

pull-request-size bot added size/L and removed size/M labels Jan 28, 2021

Format code

7a9ec4c

pull-request-size bot added size/XXL and removed size/L labels Jan 28, 2021

Revert CAKE version to make CodeFileSanity work

7eae7b1

peppy added 2 commits January 28, 2021 19:19

Revert "Format code"

71c027c

This reverts commit 7a9ec4c.

Revert dotnet format bump

9e5d8b4

pull-request-size bot added size/L and removed size/XXL labels Jan 28, 2021

Remove remaining using statement

815c48c

hez2010 added 2 commits January 29, 2021 00:35

Merge branch 'master' into dependencies

72d575a

Tracking fixes

e1f2911

pull-request-size bot added size/L and removed size/M labels Jan 28, 2021

hez2010 added 3 commits January 29, 2021 02:20

Fix deletion tracking issue

8587e30

Add workaround for deletion, fix incorrect mapping of Mods

194ae88

Removed unused usings

4d053a6

bdach reviewed Jan 28, 2021

View reviewed changes

hez2010 added 8 commits January 29, 2021 04:35

AsNoTracking

ac49f7c

Remove redundant comments

5d4cd56

Revert to use same transcation but make sure FileInfo is consistent

e196e9f

Detech unchanged entries

d04fb7d

Fix all tracking issues by only keep latest entries before add them

abc25f8

Keep latest FileInfo entries for ScoreInfo only

36d1644

Fix typo

ae6b57e

Merge branch 'master' into dependencies

3e5919b

lint fixes

5df8b75

hez2010 requested a review from bdach January 29, 2021 14:49

bdach reviewed Jan 29, 2021

View reviewed changes

peppy requested changes Feb 1, 2021

View reviewed changes

peppy closed this Mar 15, 2021

Upgrade all dependencies #11612

Upgrade all dependencies #11612

Conversation

hez2010 commented Jan 27, 2021 • edited Loading

bdach commented Jan 27, 2021

bdach left a comment

Choose a reason for hiding this comment

hez2010 commented Jan 27, 2021

peppy commented Jan 27, 2021

peppy commented Jan 28, 2021

peppy commented Jan 28, 2021

hez2010 commented Jan 28, 2021

peppy commented Jan 28, 2021

hez2010 commented Jan 28, 2021 • edited Loading

hez2010 commented Jan 28, 2021 • edited Loading

bdach commented Jan 28, 2021

peppy commented Jan 28, 2021

hez2010 commented Jan 28, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bdach commented Jan 28, 2021

hez2010 commented Jan 29, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hez2010 Jan 30, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hez2010 Feb 1, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hez2010 Feb 2, 2021 • edited Loading

Choose a reason for hiding this comment

hez2010 commented Feb 2, 2021 • edited Loading

peppy commented Feb 9, 2021

hez2010 commented Feb 9, 2021 • edited Loading

hez2010 commented Mar 1, 2021 • edited Loading

peppy commented Mar 1, 2021

peppy commented Mar 15, 2021

hez2010 commented Jan 27, 2021 •

edited

Loading

hez2010 commented Jan 28, 2021 •

edited

Loading

hez2010 commented Jan 28, 2021 •

edited

Loading

hez2010 commented Jan 28, 2021 •

edited

Loading

hez2010 commented Jan 29, 2021 •

edited

Loading

hez2010 Jan 30, 2021 •

edited

Loading

hez2010 Feb 1, 2021 •

edited

Loading

hez2010 Feb 2, 2021 •

edited

Loading

hez2010 commented Feb 2, 2021 •

edited

Loading

hez2010 commented Feb 9, 2021 •

edited

Loading

hez2010 commented Mar 1, 2021 •

edited

Loading