From b6805a94a3802aa621e7fe22dc1fe0691a295a16 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=81ukasz=20Domeradzki?= Date: Fri, 16 Aug 2024 03:25:58 +0200 Subject: [PATCH] Add workaround for LINQ race condition with concurrent collections This is some next-level race condition, so for those interested: - Concurrent collections are thread-safe in a way that each operation is atomic - Naturally if you call two atomic operations in a row, the result is no longer atomic, since there could be some changes between the first and the last - Certain LINQ operations such as OrderBy(), Reverse(), ToArray(), among more, use internal buffer for operation with certain optimization that checks if input is ICollection, if yes, it calls Count and CopyTo(), for OrderBy in this example - In result, such LINQ call is not guaranteed to be thread-safe, since it assumes those two calls to be atomic, while they're not in reality. This issue is quite hard to spot in real applications, since it's not that easy to trigger it (you need to call the operation on ICollection and then have another thread modifying it while enumerating). This is probably why we've never had any real problem until I've discovered this madness with @Aareksio in entirely different project. As a workaround, we'll explicitly convert some ICollection inputs to IEnumerable, in particular around OrderBy(), so the optimization is skipped and the result is not corrupted. I've added unit tests which ensure this workaround works properly, and you can easily reproduce the problem by removing AsLinqThreadSafeEnumerable() in them. See https://github.com/dotnet/runtime/discussions/50687 for more insight I have no clue who thought that ignoring this issue is a good idea, at the very least concurrent collections should have opt-out mechanism from those optimizations, there is no reason for them to not do that. --- ArchiSteamFarm.Tests/ConcurrentTests.cs | 93 +++++++++++++++++++++++ ArchiSteamFarm/Core/Utilities.cs | 8 ++ ArchiSteamFarm/Steam/Bot.cs | 8 +- ArchiSteamFarm/Steam/Cards/CardsFarmer.cs | 4 +- 4 files changed, 107 insertions(+), 6 deletions(-) create mode 100644 ArchiSteamFarm.Tests/ConcurrentTests.cs diff --git a/ArchiSteamFarm.Tests/ConcurrentTests.cs b/ArchiSteamFarm.Tests/ConcurrentTests.cs new file mode 100644 index 000000000..35e26f014 --- /dev/null +++ b/ArchiSteamFarm.Tests/ConcurrentTests.cs @@ -0,0 +1,93 @@ +// ---------------------------------------------------------------------------------------------- +// _ _ _ ____ _ _____ +// / \ _ __ ___ | |__ (_)/ ___| | |_ ___ __ _ _ __ ___ | ___|__ _ _ __ _ __ ___ +// / _ \ | '__|/ __|| '_ \ | |\___ \ | __|/ _ \ / _` || '_ ` _ \ | |_ / _` || '__|| '_ ` _ \ +// / ___ \ | | | (__ | | | || | ___) || |_| __/| (_| || | | | | || _|| (_| || | | | | | | | +// /_/ \_\|_| \___||_| |_||_||____/ \__|\___| \__,_||_| |_| |_||_| \__,_||_| |_| |_| |_| +// ---------------------------------------------------------------------------------------------- +// | +// Copyright 2015-2024 Łukasz "JustArchi" Domeradzki +// Contact: JustArchi@JustArchi.net +// | +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// | +// http://www.apache.org/licenses/LICENSE-2.0 +// | +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +using System.Collections.Concurrent; +using System.Collections.Generic; +using System.Linq; +using ArchiSteamFarm.Collections; +using ArchiSteamFarm.Core; +using Microsoft.VisualStudio.TestTools.UnitTesting; + +namespace ArchiSteamFarm.Tests; + +#pragma warning disable CA1812 // False positive, the class is used during MSTest +[TestClass] +internal sealed class ConcurrentTests { + [TestMethod] + internal void ConcurrentDictionarySupportsWritingDuringLinq() { + ConcurrentDictionary collection = []; + + for (byte i = 0; i < 10; i++) { + collection.TryAdd(i, true); + } + + Utilities.InBackground( + () => { + for (ushort i = 10; i < ushort.MaxValue; i++) { + collection.TryAdd(i, true); + } + }, true + ); + + foreach (KeyValuePair _ in collection.AsLinqThreadSafeEnumerable().OrderBy(static entry => entry.Key)) { } + } + + [TestMethod] + internal void ConcurrentHashSetSupportsWritingDuringLinq() { + ConcurrentHashSet collection = []; + + for (byte i = 0; i < 10; i++) { + collection.Add(i); + } + + Utilities.InBackground( + () => { + for (ushort i = 10; i < ushort.MaxValue; i++) { + collection.Add(i); + } + }, true + ); + + foreach (ushort _ in collection.AsLinqThreadSafeEnumerable().OrderBy(static entry => entry)) { } + } + + [TestMethod] + internal void ConcurrentListSupportsWritingDuringLinq() { + ConcurrentList collection = []; + + for (byte i = 0; i < 10; i++) { + collection.Add(i); + } + + Utilities.InBackground( + () => { + for (ushort i = 10; i < ushort.MaxValue; i++) { + collection.Add(i); + } + }, true + ); + + foreach (ushort _ in collection.AsLinqThreadSafeEnumerable().OrderBy(static entry => entry)) { } + } +} +#pragma warning restore CA1812 // False positive, the class is used during MSTest diff --git a/ArchiSteamFarm/Core/Utilities.cs b/ArchiSteamFarm/Core/Utilities.cs index c2a8801e3..314a8368f 100644 --- a/ArchiSteamFarm/Core/Utilities.cs +++ b/ArchiSteamFarm/Core/Utilities.cs @@ -55,6 +55,14 @@ public static class Utilities { private static readonly FrozenSet DirectorySeparators = new HashSet(2) { Path.DirectorySeparatorChar, Path.AltDirectorySeparatorChar }.ToFrozenSet(); + [PublicAPI] + public static IEnumerable AsLinqThreadSafeEnumerable(this IEnumerable enumerable) { + ArgumentNullException.ThrowIfNull(enumerable); + + // See: https://github.com/dotnet/runtime/discussions/50687 + return enumerable.Select(static entry => entry); + } + [PublicAPI] public static string GenerateChecksumFor(byte[] source) { ArgumentNullException.ThrowIfNull(source); diff --git a/ArchiSteamFarm/Steam/Bot.cs b/ArchiSteamFarm/Steam/Bot.cs index 185f59b81..8e5755d87 100644 --- a/ArchiSteamFarm/Steam/Bot.cs +++ b/ArchiSteamFarm/Steam/Bot.cs @@ -572,7 +572,7 @@ public sealed class Bot : IAsyncDisposable, IDisposable { case "@ALL": case SharedInfo.ASF: // We can return the result right away, as all bots have been matched already - return Bots.OrderBy(static bot => bot.Key, BotsComparer).Select(static bot => bot.Value).ToHashSet(); + return Bots.AsLinqThreadSafeEnumerable().OrderBy(static bot => bot.Key, BotsComparer).Select(static bot => bot.Value).ToHashSet(); case "@FARMING": IEnumerable farmingBots = Bots.Where(static bot => bot.Value.CardsFarmer.NowFarming).OrderBy(static bot => bot.Key, BotsComparer).Select(static bot => bot.Value); result.UnionWith(farmingBots); @@ -604,7 +604,7 @@ public sealed class Bot : IAsyncDisposable, IDisposable { switch (botRange.Length) { case 1: // Either bot.. or ..bot - IEnumerable query = Bots.OrderBy(static bot => bot.Key, BotsComparer).Select(static bot => bot.Value); + IEnumerable query = Bots.AsLinqThreadSafeEnumerable().OrderBy(static bot => bot.Key, BotsComparer).Select(static bot => bot.Value); query = botName.StartsWith("..", StringComparison.Ordinal) ? query.TakeWhile(bot => bot != firstBot) : query.SkipWhile(bot => bot != firstBot); @@ -620,7 +620,7 @@ public sealed class Bot : IAsyncDisposable, IDisposable { Bot? lastBot = GetBot(botRange[1]); if ((lastBot != null) && (BotsComparer.Compare(firstBot.BotName, lastBot.BotName) <= 0)) { - foreach (Bot bot in Bots.OrderBy(static bot => bot.Key, BotsComparer).Select(static bot => bot.Value).SkipWhile(bot => bot != firstBot).TakeWhile(bot => bot != lastBot)) { + foreach (Bot bot in Bots.AsLinqThreadSafeEnumerable().OrderBy(static bot => bot.Key, BotsComparer).Select(static bot => bot.Value).SkipWhile(bot => bot != firstBot).TakeWhile(bot => bot != lastBot)) { result.Add(bot); } @@ -1331,7 +1331,7 @@ public sealed class Bot : IAsyncDisposable, IDisposable { return targetBot; } - return Bots.OrderBy(static bot => bot.Key, BotsComparer).Select(static bot => bot.Value).FirstOrDefault(); + return Bots.AsLinqThreadSafeEnumerable().OrderBy(static bot => bot.Key, BotsComparer).Select(static bot => bot.Value).FirstOrDefault(); } internal Task?> GetMarketableAppIDs() => ArchiWebHandler.GetAppList(); diff --git a/ArchiSteamFarm/Steam/Cards/CardsFarmer.cs b/ArchiSteamFarm/Steam/Cards/CardsFarmer.cs index e341cd216..41eb7bc29 100644 --- a/ArchiSteamFarm/Steam/Cards/CardsFarmer.cs +++ b/ArchiSteamFarm/Steam/Cards/CardsFarmer.cs @@ -811,7 +811,7 @@ public sealed class CardsFarmer : IAsyncDisposable, IDisposable { // In order to maximize efficiency, we'll take games that are closest to our HoursPlayed first // We must call ToList() here as we can't remove items while enumerating - foreach (Game game in GamesToFarm.OrderByDescending(static game => game.HoursPlayed).ToList()) { + foreach (Game game in GamesToFarm.AsLinqThreadSafeEnumerable().OrderByDescending(static game => game.HoursPlayed).ToList()) { if (!await IsPlayableGame(game).ConfigureAwait(false)) { GamesToFarm.Remove(game); @@ -1423,7 +1423,7 @@ public sealed class CardsFarmer : IAsyncDisposable, IDisposable { private async Task SortGamesToFarm() { // Put priority idling appIDs on top - IOrderedEnumerable orderedGamesToFarm = GamesToFarm.OrderByDescending(game => Bot.IsPriorityIdling(game.AppID)); + IOrderedEnumerable orderedGamesToFarm = GamesToFarm.AsLinqThreadSafeEnumerable().OrderByDescending(game => Bot.IsPriorityIdling(game.AppID)); foreach (BotConfig.EFarmingOrder farmingOrder in Bot.BotConfig.FarmingOrders) { switch (farmingOrder) {