From 1e97ed50f61fe89d112796ee2d929a8a7512a7ae Mon Sep 17 00:00:00 2001 From: Kamron Batman <3953314+kamronbatman@users.noreply.github.com> Date: Tue, 21 Jul 2026 07:51:06 -0700 Subject: [PATCH] fix: Harden Advanced Search: crash-safety, autosave, correct results & worker fixes (#2543) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary Hardens the **Advanced Search** engine (`Projects/UOContent/Engines/Advanced Search/`) — the GM entity finder that fans searches across background worker threads. A code review surfaced 14 defects (A–N), including a shard-crasher reachable from a single admin typo and a path that silently disables autosave for the rest of the shard's uptime. Each behavioral fix ships with a test. Full `UOContent.Tests` suite: **530/530 green** (21 new AdvancedSearch tests). ## Fixes ### Crash / data-loss - **A — Shard crash on a malformed Property Test.** `AdvancedSearchThreadWorker.Execute` had no `try/catch` and the worker `Thread` is foreground, so a parse throw (`Hits>abc`, `Layer=onehanded` — `Enum.Parse` was case-sensitive, `Hits>1@` — empty sub-expression indexing) terminated the process. Now: `ParseValue`/`CompareValues` use `TryParse`/`Enum.TryParse(ignoreCase)` and return no-match instead of throwing; the per-entity filter is wrapped in `try/catch` (logs + skips); empty expressions are guarded. - **C — Overlapping searches corrupt state + brick autosave.** `_threadWorkers`/`_threadId` were `static` but `DoSearch` is an instance method; a second search (double-click / two admins) stomped shared worker state and could leave a drain waiting forever on the shared `AutoResetEvent`, so `AutoSave.SavesEnabled` was never restored. Now: an `Interlocked` re-entrancy guard rejects concurrent searches. - **G — Autosave restore not guaranteed.** The restore lived only in the success callback. Now it's in a `finally` (plus an outer `catch` covering the synchronous setup and a `catch` on the drain body), so autosave + the guard are always released. ### Wrong results - **D — `@`/`|` operator precedence.** `a@b|c` evaluated as `a && (b || c)` instead of `(a && b) || c`. OR now binds looser than AND (`AdvancedSearchUtilities.EvaluateBoolean`, unit-tested). - **E — Descending sort, partial last page rendered blank** (the index decreased in descending mode and the `break` early-out killed the loop). Now a bounded `VisibleCount`-driven loop renders the last page in both directions. - **F — Deleted entities** were not skipped (ghost rows). Now `DoEntitySearch` skips `entity.Deleted`. - **N — Reference-type comparisons** threw (`Comparer.Default.Compare` on non-`IComparable`) and compared references to a string. Now equality is by value and ordering is guarded to `IComparable` (no throw). ### Worker perf / hardening - **H** busy-spin → `Thread.Yield()` in the drain; **I** `GetProperties()` cached per `Type`; **J** `HandleValidInternal` moved behind the cheap map/range/region filters; **K** worker threads are `IsBackground` + `Exit()` tolerates an already-terminated worker; **L** `_filter == null` guard; **M** consistent `Volatile` access on `_pause`/`_exit`. ### Documented - **B** — the residual worker/event-loop read race is documented on `AdvancedSearchThreadWorker`: workers read live entity state concurrently with the loop, so value-type reads may be stale-but-safe and getter exceptions are swallowed; fully eliminating it would require snapshotting entity fields on the main thread (deferred). ## Notes - New test-only seams (`TryBeginSearch`/`EndSearch`/`IsSearchInProgress`/`VisibleCount`/`TryParseValue`/`EvaluateBoolean`) are `internal` via the existing `InternalsVisibleTo("UOContent.Tests")`. - Dead `public ParseValue` removed. - `ConcurrentDictionary` for the reflection cache is intentional — these workers are genuinely parallel. --- .../RawInterpolatedStringHandlerTests.cs | 1 + .../AdvancedSearchPagingTests.cs | 17 ++ .../AdvancedSearchTypesTests.cs | 27 ++ .../AdvancedSearchUtilitiesTests.cs | 119 +++++++++ .../AdvancedSearchWorkerTests.cs | 95 +++++++ .../Tests/Utilities/TryParseTests.cs | 26 ++ .../Advanced Search/AdvancedSearchGump.cs | 166 ++++++++----- .../AdvancedSearchThreadWorker.cs | 94 +++++-- .../AdvancedSearchUtilities.cs | 231 +++++++++++++----- .../Engines/Factions/Core/Faction.cs | 32 ++- .../UOContent/Engines/Factions/Core/Town.cs | 33 ++- Projects/UOContent/Utilities/Types.cs | 150 ++++++++---- .../01-foundation-changes.md | 60 +++++ 13 files changed, 842 insertions(+), 209 deletions(-) create mode 100644 Projects/UOContent.Tests/Tests/Engines/AdvancedSearch/AdvancedSearchPagingTests.cs create mode 100644 Projects/UOContent.Tests/Tests/Engines/AdvancedSearch/AdvancedSearchTypesTests.cs create mode 100644 Projects/UOContent.Tests/Tests/Engines/AdvancedSearch/AdvancedSearchUtilitiesTests.cs create mode 100644 Projects/UOContent.Tests/Tests/Engines/AdvancedSearch/AdvancedSearchWorkerTests.cs diff --git a/Projects/Server.Tests/Tests/Buffers/RawInterpolatedStringHandlerTests.cs b/Projects/Server.Tests/Tests/Buffers/RawInterpolatedStringHandlerTests.cs index 0b76d3f94..d4777b758 100644 --- a/Projects/Server.Tests/Tests/Buffers/RawInterpolatedStringHandlerTests.cs +++ b/Projects/Server.Tests/Tests/Buffers/RawInterpolatedStringHandlerTests.cs @@ -4,6 +4,7 @@ using Xunit; namespace Server.Tests.Buffers; +[Collection("Sequential Server Tests")] public class RawInterpolatedStringHandlerTests { [Fact] diff --git a/Projects/UOContent.Tests/Tests/Engines/AdvancedSearch/AdvancedSearchPagingTests.cs b/Projects/UOContent.Tests/Tests/Engines/AdvancedSearch/AdvancedSearchPagingTests.cs new file mode 100644 index 000000000..5166124e5 --- /dev/null +++ b/Projects/UOContent.Tests/Tests/Engines/AdvancedSearch/AdvancedSearchPagingTests.cs @@ -0,0 +1,17 @@ +using Server.Engines.AdvancedSearch; +using Xunit; + +namespace UOContent.Tests; + +public class AdvancedSearchPagingTests +{ + [Theory] + [InlineData(20, 0, 18, 18)] // full first page + [InlineData(20, 18, 18, 2)] // partial last page -> 2 visible (bug rendered 0 in descending) + [InlineData(5, 0, 18, 5)] + [InlineData(0, 0, 18, 0)] + public void VisibleCount_IsCorrect(int total, int from, int max, int expected) + { + Assert.Equal(expected, AdvancedSearchGump.VisibleCount(total, from, max)); + } +} diff --git a/Projects/UOContent.Tests/Tests/Engines/AdvancedSearch/AdvancedSearchTypesTests.cs b/Projects/UOContent.Tests/Tests/Engines/AdvancedSearch/AdvancedSearchTypesTests.cs new file mode 100644 index 000000000..ea14f3cf2 --- /dev/null +++ b/Projects/UOContent.Tests/Tests/Engines/AdvancedSearch/AdvancedSearchTypesTests.cs @@ -0,0 +1,27 @@ +using Server; +using Server.Engines.AdvancedSearch; +using Xunit; + +namespace UOContent.Tests; + +[Collection("Sequential UOContent Tests")] +public class AdvancedSearchTypesTests +{ + [Fact] + public void CompareValues_Poison_ReferenceTypeParsedViaTypes() + { + PoisonKinds.Configure(); // idempotent; registers Lesser..Lethal now that Core.Expansion is set + + // Poison is a reference type implementing ISpanParsable; it can't use the compile-time span + // path and routes through the shared Server.Types converter. Poison.Parse returns the + // registered singleton, so "= Lethal" is a reference-equality match — this is the case that + // previously compared a Poison against the raw string and always failed. + var prop = Poison.Lethal; + Assert.True(AdvancedSearchUtilities.CompareValues(typeof(Poison), prop, "Lethal", "=")); + Assert.False(AdvancedSearchUtilities.CompareValues(typeof(Poison), prop, "Lesser", "=")); + + var ex = Record.Exception(() => + Assert.False(AdvancedSearchUtilities.CompareValues(typeof(Poison), prop, "notapoison", "="))); + Assert.Null(ex); + } +} diff --git a/Projects/UOContent.Tests/Tests/Engines/AdvancedSearch/AdvancedSearchUtilitiesTests.cs b/Projects/UOContent.Tests/Tests/Engines/AdvancedSearch/AdvancedSearchUtilitiesTests.cs new file mode 100644 index 000000000..9176fae4a --- /dev/null +++ b/Projects/UOContent.Tests/Tests/Engines/AdvancedSearch/AdvancedSearchUtilitiesTests.cs @@ -0,0 +1,119 @@ +using System; +using Server; +using Server.Engines.AdvancedSearch; +using Xunit; + +namespace UOContent.Tests; + +public class AdvancedSearchUtilitiesTests +{ + [Theory] + [InlineData("abc")] // not a number -> was FormatException + [InlineData("99999999999")] // overflows int -> was OverflowException + [InlineData("0xZZ")] // bad hex -> was FormatException + public void CompareValues_BadNumeric_ReturnsFalse_DoesNotThrow(string value) + { + var ex = Record.Exception(() => + { + var result = AdvancedSearchUtilities.CompareValues(typeof(int), 5, value, ">"); + Assert.False(result); + }); + Assert.Null(ex); + } + + [Theory] + [InlineData("Bogus")] // not a member -> was ArgumentException + [InlineData("onehandedxyz")] // not a member, even case-insensitively -> was ArgumentException + public void CompareValues_BadEnum_ReturnsFalse_DoesNotThrow(string value) + { + var ex = Record.Exception(() => + { + var result = AdvancedSearchUtilities.CompareValues(typeof(Layer), (byte)Layer.OneHanded, value, "="); + Assert.False(result); + }); + Assert.Null(ex); + } + + [Fact] + public void CompareValues_ValidEnum_IgnoresCase() + { + Assert.True(AdvancedSearchUtilities.CompareValues(typeof(Layer), (byte)Layer.OneHanded, "onehanded", "=")); + } + + [Theory] + // leaf value is "T"/"F"; evalLeaf returns leaf=="T" + [InlineData("T", true)] + [InlineData("F", false)] + [InlineData("F@F|T", true)] // (F&&F)||T = T (buggy code gave F&&(F||T)=F) + [InlineData("T|F@F", true)] // T||(F&&F) = T (buggy code gave (T||F)&&F=F) + [InlineData("T@F", false)] + [InlineData("T@T", true)] + [InlineData("F|F", false)] + public void EvaluateBoolean_Precedence(string expr, bool expected) + { + // State is unused here; the leaf evaluator just checks the span equals "T". + var result = AdvancedSearchUtilities.EvaluateBoolean(expr, 0, static (_, leaf) => leaf.SequenceEqual("T")); + Assert.Equal(expected, result); + } + + [Fact] + public void CompareValues_ReferenceType_EqualityByString_NoThrow() + { + // A reference-typed property (e.g. RootParent name-ish) compared with "=" should not throw, + // and ordering operators must return false rather than throwing. + var ex = Record.Exception(() => + { + Assert.False(AdvancedSearchUtilities.CompareValues(typeof(object), new object(), "whatever", ">")); + }); + Assert.Null(ex); + } + + [Fact] + public void CompareValues_TimeSpan_ParsesViaSpanParsable() + { + // TimeSpan is not IConvertible, so the old Convert.ChangeType fallback threw and silently + // returned no-match. ISpanParsable parses it correctly. + var prop = TimeSpan.FromMinutes(5); + Assert.True(AdvancedSearchUtilities.CompareValues(typeof(TimeSpan), prop, "00:05:00", "=")); + Assert.False(AdvancedSearchUtilities.CompareValues(typeof(TimeSpan), prop, "00:10:00", "=")); + Assert.True(AdvancedSearchUtilities.CompareValues(typeof(TimeSpan), prop, "00:01:00", ">")); + } + + [Fact] + public void CompareValues_TimeSpan_BadInput_ReturnsFalse_NoThrow() + { + var ex = Record.Exception(() => + Assert.False(AdvancedSearchUtilities.CompareValues(typeof(TimeSpan), TimeSpan.Zero, "notaspan", "="))); + Assert.Null(ex); + } + + [Fact] + public void CompareValues_Guid_ValueTypeParsedViaTypes() + { + // Guid is a value type not named by the hot paths; it's parsed via Types (IParsable) and + // compared by value. + var g = Guid.Parse("00000000-0000-0000-0000-000000000001"); + Assert.True(AdvancedSearchUtilities.CompareValues(typeof(Guid), g, "00000000-0000-0000-0000-000000000001", "=")); + Assert.False(AdvancedSearchUtilities.CompareValues(typeof(Guid), g, "00000000-0000-0000-0000-000000000002", "=")); + } + + // A reference type with a legacy RunUO-style static Parse(string) and NO IParsable<> interface — + // the Faction/Town shape. Types must still discover its Parse by reflection. + private sealed class LegacyParseType + { + public string Value { get; private init; } + public static LegacyParseType Parse(string s) => new() { Value = s }; + public override bool Equals(object obj) => obj is LegacyParseType o && o.Value == Value; + public override int GetHashCode() => Value?.GetHashCode() ?? 0; + } + + [Fact] + public void CompareValues_LegacyParseString_ParsedViaTypes() + { + // Pre-IParsable types (only a static Parse(string)) must still be searchable: Types binds the + // legacy Parse by reflection, so we compare against a real parsed instance, not the raw text. + var prop = LegacyParseType.Parse("alpha"); + Assert.True(AdvancedSearchUtilities.CompareValues(typeof(LegacyParseType), prop, "alpha", "=")); + Assert.False(AdvancedSearchUtilities.CompareValues(typeof(LegacyParseType), prop, "beta", "=")); + } +} diff --git a/Projects/UOContent.Tests/Tests/Engines/AdvancedSearch/AdvancedSearchWorkerTests.cs b/Projects/UOContent.Tests/Tests/Engines/AdvancedSearch/AdvancedSearchWorkerTests.cs new file mode 100644 index 000000000..2bfe94af8 --- /dev/null +++ b/Projects/UOContent.Tests/Tests/Engines/AdvancedSearch/AdvancedSearchWorkerTests.cs @@ -0,0 +1,95 @@ +using System; +using System.Collections.Concurrent; +using Server; +using Server.Engines.AdvancedSearch; +using Server.Items; +using Server.Tests; +using Xunit; + +namespace UOContent.Tests; + +[Collection("Sequential UOContent Tests")] +public class AdvancedSearchWorkerTests +{ + // An item whose property test path will throw when evaluated. + private sealed class ThrowingItem : Item + { + public ThrowingItem() : base(0x1) { } + public ThrowingItem(Serial s) : base(s) { } + public string Boom => throw new InvalidOperationException("boom"); + } + + [Fact] + public void Worker_FilterThrows_DoesNotEscape_ReturnsNoMatch() + { + var worker = new AdvancedSearchThreadWorker(); + var results = new ConcurrentQueue(); + var ignore = new ConcurrentQueue(); + var filter = new AdvancedSearchFilter + { + FilterPropertyTest = true, + PropertyTest = "Boom=1", // reflection GetValue -> throws + }; + + var item = new ThrowingItem(); + + try + { + worker.Wake(new WorldLocation(Point3D.Zero, Map.Felucca), filter, results, ignore); + worker.Push(item); + worker.Sleep(); // drains; must not crash the test process + + Assert.Empty(results); + } + finally + { + item.Delete(); + worker.Exit(); + } + } + + [Fact] + public void Worker_DeletedEntity_IsSkipped() + { + var worker = new AdvancedSearchThreadWorker(); + var results = new ConcurrentQueue(); + var ignore = new ConcurrentQueue(); + var filter = new AdvancedSearchFilter(); // no filters -> everything matches + + var item = new Item(0x1); + item.Delete(); + + try + { + worker.Wake(new WorldLocation(Point3D.Zero, Map.Felucca), filter, results, ignore); + worker.Push(item); + worker.Sleep(); + + Assert.Empty(results); + } + finally + { + worker.Exit(); + } + } + + [Fact] + public void DoSearch_IsGuarded_AgainstReentry() + { + // White-box: flip the guard, assert a second entry is rejected, then clear. + // _searchInProgress is process-global static state; release it in finally so a + // failed assert here can't leak the guard into other tests. + Assert.False(AdvancedSearchGump.IsSearchInProgress); + Assert.True(AdvancedSearchGump.TryBeginSearch()); // acquires + try + { + Assert.False(AdvancedSearchGump.TryBeginSearch()); // rejected + } + finally + { + AdvancedSearchGump.EndSearch(); // releases + } + + Assert.False(AdvancedSearchGump.IsSearchInProgress); + } +} diff --git a/Projects/UOContent.Tests/Tests/Utilities/TryParseTests.cs b/Projects/UOContent.Tests/Tests/Utilities/TryParseTests.cs index fa6de64a5..314290264 100644 --- a/Projects/UOContent.Tests/Tests/Utilities/TryParseTests.cs +++ b/Projects/UOContent.Tests/Tests/Utilities/TryParseTests.cs @@ -1,3 +1,4 @@ +using System; using Xunit; namespace Server.Tests.Utility; @@ -18,4 +19,29 @@ public class TryParseTests Assert.Equal(parsedAs, constructed); } } + + [Theory] + // Parsed directly into the target type (INumber.TryParse), not via ulong + Convert.ChangeType. + [InlineData(typeof(int), "42", true, 42)] + [InlineData(typeof(int), "-5", true, -5)] // signed values parse directly now + [InlineData(typeof(int), "0xFF", true, 255)] // hex + [InlineData(typeof(int), "notanumber", false, null)] + [InlineData(typeof(byte), "255", true, (byte)255)] + [InlineData(typeof(byte), "256", false, null)] // out of the byte range + [InlineData(typeof(uint), "4294967295", true, 4294967295u)] + [InlineData(typeof(long), "-9000000000", true, -9000000000L)] + public void TestTryParseNumeric(Type type, string value, bool success, object expected) + { + var error = Server.Types.TryParse(type, value, out var constructed); + + if (success) + { + Assert.Null(error); + Assert.Equal(expected, constructed); + } + else + { + Assert.NotNull(error); + } + } } diff --git a/Projects/UOContent/Engines/Advanced Search/AdvancedSearchGump.cs b/Projects/UOContent/Engines/Advanced Search/AdvancedSearchGump.cs index 58a7f73a9..7f72c02b2 100644 --- a/Projects/UOContent/Engines/Advanced Search/AdvancedSearchGump.cs +++ b/Projects/UOContent/Engines/Advanced Search/AdvancedSearchGump.cs @@ -8,6 +8,7 @@ using Server.Commands; using Server.Commands.Generic; using Server.Engines.Spawners; using Server.Gumps; +using Server.Logging; using Server.Network; using Server.Saves; @@ -28,11 +29,19 @@ public enum AdvancedSearchGumpOptions : long public class AdvancedSearchGump : Gump { + private static readonly ILogger _logger = LogFactory.GetLogger(typeof(AdvancedSearchGump)); + private const int MaxEntries = 18; private static int _threadId; private static AdvancedSearchThreadWorker[] _threadWorkers; + private static int _searchInProgress; + + internal static bool IsSearchInProgress => Volatile.Read(ref _searchInProgress) == 1; + internal static bool TryBeginSearch() => Interlocked.CompareExchange(ref _searchInProgress, 1, 0) == 0; + internal static void EndSearch() => Volatile.Write(ref _searchInProgress, 0); + private static void Configure() { EventSink.Shutdown += Shutdown; @@ -124,6 +133,10 @@ public class AdvancedSearchGump : Gump public AdvancedSearchGump() : base(50, 50) => Build(); + // Entries on the current page — bounds the paging loop so a partial last page can't read past the end. + internal static int VisibleCount(int total, int displayFrom, int maxEntries) => + Math.Clamp(total - displayFrom, 0, maxEntries); + private void Build() { const int height = 500; @@ -319,15 +332,13 @@ public class AdvancedSearchGump : Gump var allDisplayedSelected = true; - for (var i = 0; i < MaxEntries; i++) - { - var offset = SortDescending ? MaxEntries - 1 - i : i; - var index = offset + DisplayFrom; + // Bound to this page's real entries so a partial last page still renders in descending mode. + var visibleCount = VisibleCount(SearchResults.Length, DisplayFrom, MaxEntries); - if (index >= SearchResults.Length) - { - break; - } + for (var i = 0; i < visibleCount; i++) + { + var offset = SortDescending ? visibleCount - 1 - i : i; + var index = offset + DisplayFrom; var entry = SearchResults[index]; @@ -739,78 +750,103 @@ public class AdvancedSearchGump : Gump return; } + if (!TryBeginSearch()) + { + from.SendMessage("A search is already running. Please wait for it to finish."); + return; + } + + _threadId = 0; + var autoSave = AutoSave.SavesEnabled; - if (autoSave) + AutoSave.SavesEnabled = false; + + try { - AutoSave.SavesEnabled = false; - } + _threadWorkers ??= new AdvancedSearchThreadWorker[Math.Max(Environment.ProcessorCount - 1, 1)]; - _threadWorkers ??= new AdvancedSearchThreadWorker[Math.Max(Environment.ProcessorCount - 1, 1)]; + var ignoreQueue = new ConcurrentQueue(); + var results = new ConcurrentQueue(); + var worldLocation = new WorldLocation(from.Location, from.Map); - var ignoreQueue = new ConcurrentQueue(); - var results = new ConcurrentQueue(); - var worldLocation = new WorldLocation(from.Location, from.Map); - - for (var i = 0; i < _threadWorkers.Length; i++) - { - (_threadWorkers[i] ??= new AdvancedSearchThreadWorker()).Wake(worldLocation, Filter, results, ignoreQueue); - } - - var type = Filter.FilterType ? Filter.Type : null; - - // Push the entities - foreach (var item in World.Items.Values) - { - if (type == null || type.IsInstanceOfType(item)) - { - PushToWorkers(item); - } - } - - foreach (var m in World.Mobiles.Values) - { - if (type == null || type.IsInstanceOfType(m)) - { - PushToWorkers(m); - } - } - - ThreadPool.QueueUserWorkItem(state => - { - // Block until everything is processed for (var i = 0; i < _threadWorkers.Length; i++) { - _threadWorkers[i].Sleep(); + (_threadWorkers[i] ??= new AdvancedSearchThreadWorker()).Wake(worldLocation, Filter, results, ignoreQueue); } - var ignoredEntities = new HashSet(ignoreQueue); + var type = Filter.FilterType ? Filter.Type : null; - // Force the GC to collect the ignored entities - ignoreQueue.Clear(); - - var resultsList = new List(results.Count); - foreach (var result in results) + // Push the entities. Workers read entity state concurrently with the main loop — + // see AdvancedSearchThreadWorker for the accepted, bounded race. + foreach (var item in World.Items.Values) { - if (!ignoredEntities.Contains(result.Entity)) + if (type == null || type.IsInstanceOfType(item)) { - resultsList.Add(result); + PushToWorkers(item); } } - SearchResults = resultsList.ToArray(); - - // Force the GC to collect the results - resultsList.Clear(); - resultsList.TrimExcess(); - - // Send the gump on the main thread - Core.LoopContext.Post( - autoSaveState => + foreach (var m in World.Mobiles.Values) + { + if (type == null || type.IsInstanceOfType(m)) { - AutoSave.SavesEnabled = (bool)autoSaveState!; - Resend(from); - }, state); - }, autoSave); + PushToWorkers(m); + } + } + + ThreadPool.QueueUserWorkItem(state => + { + try + { + // Block until everything is processed + for (var i = 0; i < _threadWorkers.Length; i++) + { + _threadWorkers[i].Sleep(); + } + + var ignoredEntities = new HashSet(ignoreQueue); + + // Force the GC to collect the ignored entities + ignoreQueue.Clear(); + + var resultsList = new List(results.Count); + foreach (var result in results) + { + if (!ignoredEntities.Contains(result.Entity)) + { + resultsList.Add(result); + } + } + + SearchResults = resultsList.ToArray(); + + // Force the GC to collect the results + resultsList.Clear(); + resultsList.TrimExcess(); + + // Send the gump on the main thread + Core.LoopContext.Post(() => Resend(from)); + } + catch (Exception ex) + { + // A drain-phase throw here would terminate the process; the finally still + // restores autosave and releases the guard. + _logger.Warning(ex, "AdvancedSearch: search drain failed"); + } + finally + { + AutoSave.SavesEnabled = (bool)state!; + EndSearch(); + } + }, autoSave); + } + catch + { + // Setup failed before the work item took ownership of the release. + AutoSave.SavesEnabled = autoSave; + EndSearch(); + throw; + } } private void SetSortSwitches(int radioSwitch) diff --git a/Projects/UOContent/Engines/Advanced Search/AdvancedSearchThreadWorker.cs b/Projects/UOContent/Engines/Advanced Search/AdvancedSearchThreadWorker.cs index d63c0a442..a4a26c244 100644 --- a/Projects/UOContent/Engines/Advanced Search/AdvancedSearchThreadWorker.cs +++ b/Projects/UOContent/Engines/Advanced Search/AdvancedSearchThreadWorker.cs @@ -3,13 +3,26 @@ using System.Collections.Concurrent; using System.Reflection; using System.Threading; using Server.Items; +using Server.Logging; using Server.Mobiles; using Server.Multis; namespace Server.Engines.AdvancedSearch; +/// +/// Filters entities on a background thread while the main loop keeps mutating them — an +/// intentional, bounded race. Reads of live / state are +/// unsynchronized, so a torn read may report stale coordinates, and any +/// getter that throws mid-read is caught per-entity in and skipped. +/// Results are best-effort and may omit a concurrently modified entity, but never fault or corrupt +/// server state. Eliminating the race would require snapshotting each read field onto the main +/// thread before handing entities off; that is deferred. +/// public class AdvancedSearchThreadWorker { + private static readonly ILogger _logger = LogFactory.GetLogger(typeof(AdvancedSearchThreadWorker)); + private static readonly ConcurrentDictionary _propCache = new(); + private readonly Thread _thread; private readonly AutoResetEvent _startEvent; // Main thread tells the thread to start working private readonly AutoResetEvent _stopEvent; // Main thread waits for the worker finish draining @@ -26,7 +39,10 @@ public class AdvancedSearchThreadWorker _startEvent = new AutoResetEvent(false); _stopEvent = new AutoResetEvent(false); _entities = new ConcurrentQueue(); - _thread = new Thread(Execute); + _thread = new Thread(Execute) + { + IsBackground = true + }; _thread.Start(this); } @@ -52,10 +68,16 @@ public class AdvancedSearchThreadWorker public void Exit() { - _exit = true; + Volatile.Write(ref _exit, true); Wake(WorldLocation.Zero, null, null, null); - Sleep(); + + // Tolerate a worker that has already terminated (e.g. Core.Closing raced us) so + // shutdown can't deadlock waiting on a stopEvent that will never be set. + if (_thread.IsAlive) + { + Sleep(); + } } public void Push(IEntity entity) @@ -88,12 +110,17 @@ public class AdvancedSearchThreadWorker worker._filter = null; break; } + else + { + // Transiently empty but not yet paused: yield rather than busy-spin. + Thread.Yield(); + } } worker._stopEvent.Set(); // Allow the main thread to continue now that we are finished - worker._pause = false; + Volatile.Write(ref worker._pause, false); - if (Core.Closing || worker._exit) + if (Core.Closing || Volatile.Read(ref worker._exit)) { return; } @@ -102,9 +129,28 @@ public class AdvancedSearchThreadWorker private AdvancedSearchResult DoEntitySearch(IEntity entity) { - if (_filter.HideValidInternalMap) + if (entity == null || entity.Deleted) { - HandleValidInternal(entity); + return null; + } + + try + { + return DoEntitySearchCore(entity); + } + catch (Exception ex) + { + _logger.Warning(ex, "AdvancedSearch: filter threw for {Entity}; skipping", entity); + return null; + } + } + + private AdvancedSearchResult DoEntitySearchCore(IEntity entity) + { + if (_filter == null) + { + // Exit() clears the filter; a straggler entity dequeued after teardown bails here. + return null; } // Check for valid map @@ -138,6 +184,12 @@ public class AdvancedSearchThreadWorker return null; } + // After the cheap filters, so non-qualifying entities skip the house/keyring enumeration. + if (_filter.HideValidInternalMap) + { + HandleValidInternal(entity); + } + if (entity is Mobile mobile) { return DoMobileSearch(mobile); @@ -296,27 +348,17 @@ public class AdvancedSearchThreadWorker } } - private static bool EvaluateRecursive(IEntity entity, ReadOnlySpan span) - { - var atIndex = span.IndexOf('@'); - var orIndex = span.IndexOf('|'); - - if (atIndex == -1 && orIndex == -1) - { - return EvaluateSingleExpression(entity, span); - } - - var result = atIndex != -1; - var splitIndex = result ? atIndex : orIndex; - - var left = EvaluateRecursive(entity, span.Slice(0, splitIndex)); - var right = EvaluateRecursive(entity, span.Slice(splitIndex + 1)); - - return result ? left && right : left || right; - } + private static bool EvaluateRecursive(IEntity entity, ReadOnlySpan span) => + AdvancedSearchUtilities.EvaluateBoolean(span, entity, static (e, leaf) => EvaluateSingleExpression(e, leaf)); private static bool EvaluateSingleExpression(IEntity entity, ReadOnlySpan expression) { + expression = expression.Trim(); + if (expression.Length == 0) + { + return false; + } + var negate = false; if (expression[0] == '~') { @@ -338,7 +380,7 @@ public class AdvancedSearchThreadWorker return false; } - var properties = entity.GetType().GetProperties(); + var properties = _propCache.GetOrAdd(entity.GetType(), static t => t.GetProperties()); PropertyInfo property = null; for (var i = 0; i < properties.Length; ++i) { diff --git a/Projects/UOContent/Engines/Advanced Search/AdvancedSearchUtilities.cs b/Projects/UOContent/Engines/Advanced Search/AdvancedSearchUtilities.cs index 8d7dae3c4..2c349847d 100644 --- a/Projects/UOContent/Engines/Advanced Search/AdvancedSearchUtilities.cs +++ b/Projects/UOContent/Engines/Advanced Search/AdvancedSearchUtilities.cs @@ -43,77 +43,80 @@ public static class AdvancedSearchUtilities if (propertyType == typeof(long)) { - var parsedValue = ParseValue(valuePart); - return CompareNumeric((long)propertyValue!, parsedValue, operatorSpan); + return TryParseValue(valuePart, out var parsedValue) && + CompareNumeric((long)propertyValue!, parsedValue, operatorSpan); } if (propertyType == typeof(ulong)) { - var parsedValue = ParseValue(valuePart); - return CompareNumeric((ulong)propertyValue!, parsedValue, operatorSpan); + return TryParseValue(valuePart, out var parsedValue) && + CompareNumeric((ulong)propertyValue!, parsedValue, operatorSpan); } if (propertyType == typeof(int)) { - var parsedValue = ParseValue(valuePart); - return CompareNumeric((int)propertyValue!, parsedValue, operatorSpan); + return TryParseValue(valuePart, out var parsedValue) && + CompareNumeric((int)propertyValue!, parsedValue, operatorSpan); } if (propertyType == typeof(uint)) { - var parsedValue = ParseValue(valuePart); - return CompareNumeric((uint)propertyValue!, parsedValue, operatorSpan); + return TryParseValue(valuePart, out var parsedValue) && + CompareNumeric((uint)propertyValue!, parsedValue, operatorSpan); } if (propertyType == typeof(short)) { - var parsedValue = ParseValue(valuePart); - return CompareNumeric((short)propertyValue!, parsedValue, operatorSpan); + return TryParseValue(valuePart, out var parsedValue) && + CompareNumeric((short)propertyValue!, parsedValue, operatorSpan); } if (propertyType == typeof(ushort)) { - var parsedValue = ParseValue(valuePart); - return CompareNumeric((ushort)propertyValue!, parsedValue, operatorSpan); + return TryParseValue(valuePart, out var parsedValue) && + CompareNumeric((ushort)propertyValue!, parsedValue, operatorSpan); } if (propertyType == typeof(sbyte)) { - var parsedValue = ParseValue(valuePart); - return CompareNumeric((sbyte)propertyValue!, parsedValue, operatorSpan); + return TryParseValue(valuePart, out var parsedValue) && + CompareNumeric((sbyte)propertyValue!, parsedValue, operatorSpan); } if (propertyType == typeof(byte)) { - var parsedValue = ParseValue(valuePart); - return CompareNumeric((byte)propertyValue!, parsedValue, operatorSpan); + return TryParseValue(valuePart, out var parsedValue) && + CompareNumeric((byte)propertyValue!, parsedValue, operatorSpan); } if (propertyType == typeof(float)) { - var parsedValue = ParseValue(valuePart); - return Compare((float)propertyValue!, parsedValue, valuePart, operatorSpan); + return TryParseValue(valuePart, out var parsedValue) && + Compare((float)propertyValue!, parsedValue, valuePart, operatorSpan); } if (propertyType == typeof(double)) { - var parsedValue = ParseValue(valuePart); - return Compare((double)propertyValue!, parsedValue, valuePart, operatorSpan); + return TryParseValue(valuePart, out var parsedValue) && + Compare((double)propertyValue!, parsedValue, valuePart, operatorSpan); } if (propertyType == typeof(string)) { - var parsedValue = ParseValue(valuePart); - return Compare((string)propertyValue!, parsedValue, operatorSpan); + return TryParseValue(valuePart, out var parsedValue) && + Compare((string)propertyValue!, parsedValue, operatorSpan); } if (propertyType == typeof(TimeSpan)) { - var parsedValue = ParseValue(valuePart); - return Compare((TimeSpan)propertyValue!, parsedValue, operatorSpan); + return TryParseValue(valuePart, out var parsedValue) && + Compare((TimeSpan)propertyValue!, parsedValue, operatorSpan); } if (propertyType == typeof(DateTime)) { - var parsedValue = ParseValue(valuePart); - return Compare((DateTime)propertyValue!, parsedValue, operatorSpan); + return TryParseValue(valuePart, out var parsedValue) && + Compare((DateTime)propertyValue!, parsedValue, operatorSpan); } if (propertyType == typeof(bool)) { - var parsedValue = ParseValue(valuePart); - return Compare((bool)propertyValue!, parsedValue, operatorSpan); + return TryParseValue(valuePart, out var parsedValue) && + Compare((bool)propertyValue!, parsedValue, operatorSpan); } if (propertyType.IsEnum) { - var valueEnum = Enum.Parse(propertyType, valuePart, false); + if (!Enum.TryParse(propertyType, valuePart.ToString(), true, out var valueEnum) || valueEnum == null) + { + return false; + } return GetEnumSize(propertyType) switch { @@ -121,15 +124,17 @@ public static class AdvancedSearchUtilities 2 => CompareNumeric((short)propertyValue!, (short)valueEnum, operatorSpan), 4 => CompareNumeric((int)propertyValue!, (int)valueEnum, operatorSpan), 8 => CompareNumeric((long)propertyValue!, (long)valueEnum, operatorSpan), + _ => false }; } - if (!propertyType.IsValueType) - { - var parsedValue = ParseValue(valuePart); - return CompareReference(propertyValue!, parsedValue, operatorSpan); - } - - return false; + // Anything the hot typed paths above didn't handle — reference types (Poison, Map, entity + // properties resolved by serial), IParsable value types (Guid, decimal, ...), and legacy + // RunUO types with a static Parse(string) (Faction, Town, ...). Delegate to the shared, + // thread-safe Types converter so the target is parsed into the property's real type, then + // compare by value. A string is allocated here, but this is the uncommon path; the common + // types never reach it. Types returns a non-null message when it can't parse -> no match. + return Types.TryParse(propertyType, valuePart.ToString(), out var parsed) == null && + CompareReference(propertyValue!, parsed, operatorSpan); } public static bool CompareNumeric(T propertyValue, T parsedValue, ReadOnlySpan operatorSpan) where T : INumber => @@ -236,19 +241,40 @@ public static class AdvancedSearchUtilities _ => false }; - public static bool CompareReference(T propertyValue, T parsedValue, ReadOnlySpan operatorSpan) => - operatorSpan switch + public static bool CompareReference(T propertyValue, T parsedValue, ReadOnlySpan operatorSpan) + { + switch (operatorSpan) { - "=" or "==" => propertyValue.Equals(parsedValue), - "!" or "!=" => !propertyValue.Equals(parsedValue), - ">" => Comparer.Default.Compare(propertyValue, parsedValue) > 0, - "<" => Comparer.Default.Compare(propertyValue, parsedValue) < 0, - ">=" => Comparer.Default.Compare(propertyValue, parsedValue) >= 0, - "<=" => Comparer.Default.Compare(propertyValue, parsedValue) <= 0, - _ => false - }; + case "=": + case "==": return Equals(propertyValue, parsedValue); + case "!": + case "!=": return !Equals(propertyValue, parsedValue); + } - public static T ParseValue(ReadOnlySpan valuePart) + if (propertyValue is IComparable cmp && parsedValue != null) + { + try + { + var c = cmp.CompareTo(parsedValue); + return operatorSpan switch + { + ">" => c > 0, + "<" => c < 0, + ">=" => c >= 0, + "<=" => c <= 0, + _ => false + }; + } + catch + { + return false; + } + } + + return false; + } + + internal static bool TryParseValue(ReadOnlySpan valuePart, out T value) { // Special handling for boolean and hexadecimal values if (typeof(T) == typeof(bool)) @@ -256,74 +282,149 @@ public static class AdvancedSearchUtilities var val = valuePart.ToString().ToLower(); if (val is "true" or "1" or "enabled" or "on") { - return (T)(object)true; + value = (T)(object)true; + return true; } if (val is "false" or "0" or "disabled" or "off") { - return (T)(object)false; + value = (T)(object)false; + return true; } + + value = default; + return false; } if (typeof(T) == typeof(long)) { - return ParseNumericValue(valuePart); + return TryParseNumericValue(valuePart, out value); } if (typeof(T) == typeof(ulong)) { - return ParseNumericValue(valuePart); + return TryParseNumericValue(valuePart, out value); } if (typeof(T) == typeof(int)) { - return ParseNumericValue(valuePart); + return TryParseNumericValue(valuePart, out value); } if (typeof(T) == typeof(uint)) { - return ParseNumericValue(valuePart); + return TryParseNumericValue(valuePart, out value); } if (typeof(T) == typeof(short)) { - return ParseNumericValue(valuePart); + return TryParseNumericValue(valuePart, out value); } if (typeof(T) == typeof(ushort)) { - return ParseNumericValue(valuePart); + return TryParseNumericValue(valuePart, out value); } if (typeof(T) == typeof(sbyte)) { - return ParseNumericValue(valuePart); + return TryParseNumericValue(valuePart, out value); } if (typeof(T) == typeof(byte)) { - return ParseNumericValue(valuePart); + return TryParseNumericValue(valuePart, out value); } if (typeof(T) == typeof(float)) { - return ParseNumericValue(valuePart); + return TryParseNumericValue(valuePart, out value); } if (typeof(T) == typeof(double)) { - return ParseNumericValue(valuePart); + return TryParseNumericValue(valuePart, out value); } - // Default parsing for other types - return (T)Convert.ChangeType(valuePart.ToString(), typeof(T)); + // string needs no parsing — the span itself is the value. + if (typeof(T) == typeof(string)) + { + value = (T)(object)valuePart.ToString(); + return true; + } + + // Remaining supported types (TimeSpan, DateTime) parse straight from the span via + // ISpanParsable — no allocation, no reflection, and unlike Convert.ChangeType it handles + // TimeSpan, which is not IConvertible and previously failed silently. + if (typeof(T) == typeof(TimeSpan)) + { + return TryParseSpanParsable(valuePart, out value); + } + + if (typeof(T) == typeof(DateTime)) + { + return TryParseSpanParsable(valuePart, out value); + } + + value = default; + return false; + } + + // Parses U (a value type exposing ISpanParsable) from the span and reinterprets it as T. The + // two type params mirror TryParseNumericValue: the caller dispatches on typeof(T), so U == T at + // every call site and the (T)(object) cast is always valid. + [MethodImpl(MethodImplOptions.AggressiveInlining)] + private static bool TryParseSpanParsable(ReadOnlySpan valuePart, out T value) where U : ISpanParsable + { + if (U.TryParse(valuePart, null, out var parsed)) + { + value = (T)(object)parsed; + return true; + } + + value = default; + return false; } [MethodImpl(MethodImplOptions.AggressiveInlining)] - private static R ParseNumericValue(ReadOnlySpan valuePart) where T : INumber => - valuePart.StartsWith("0x") - ? (R)(object)T.Parse(valuePart[2..], NumberStyles.HexNumber, null) - : (R)(object)T.Parse(valuePart, null); + private static bool TryParseNumericValue(ReadOnlySpan valuePart, out R value) where T : INumber + { + var ok = valuePart.StartsWith("0x") + ? T.TryParse(valuePart[2..], NumberStyles.HexNumber, null, out var parsed) + : T.TryParse(valuePart, null, out parsed); + + if (ok) + { + value = (R)(object)parsed; + return true; + } + + value = default; + return false; + } + + // Evaluates one trimmed leaf atom against caller-supplied state. A custom delegate is required + // because ReadOnlySpan cannot be a Func<> type argument; passing state avoids a per-call + // capturing closure, so the recursion allocates neither a string nor a closure. + internal delegate bool LeafEvaluator(TState state, ReadOnlySpan leaf); + + // OR ('|') binds looser than AND ('@'); split on the outermost OR first, then AND. + internal static bool EvaluateBoolean(ReadOnlySpan expr, TState state, LeafEvaluator evalLeaf) + { + var orIndex = expr.IndexOf('|'); + if (orIndex != -1) + { + return EvaluateBoolean(expr[..orIndex], state, evalLeaf) || EvaluateBoolean(expr[(orIndex + 1)..], state, evalLeaf); + } + + var andIndex = expr.IndexOf('@'); + if (andIndex != -1) + { + return EvaluateBoolean(expr[..andIndex], state, evalLeaf) && EvaluateBoolean(expr[(andIndex + 1)..], state, evalLeaf); + } + + return evalLeaf(state, expr.Trim()); + } private static int GetEnumSize(Type enumType) => Type.GetTypeCode(Enum.GetUnderlyingType(enumType)) switch diff --git a/Projects/UOContent/Engines/Factions/Core/Faction.cs b/Projects/UOContent/Engines/Factions/Core/Faction.cs index 8aa8085ca..33166f5a0 100644 --- a/Projects/UOContent/Engines/Factions/Core/Faction.cs +++ b/Projects/UOContent/Engines/Factions/Core/Faction.cs @@ -16,7 +16,7 @@ using Server.Targeting; namespace Server.Factions; [CustomEnum(["Minax", "Council of Mages", "True Britannians", "Shadowlords"])] -public abstract class Faction : IComparable +public abstract class Faction : IComparable, ISpanParsable { public const int StabilityFactor = 300; // 300% greater (3 times) than smallest faction public const int StabilityActivation = 200; // Stability code goes into effect when largest faction has > 200 people @@ -1313,7 +1313,27 @@ public abstract class Faction : IComparable return null; } - public static Faction Parse(string name) + [MethodImpl(MethodImplOptions.AggressiveInlining)] + public static Faction Parse(string s) => Parse(s, null); + + [MethodImpl(MethodImplOptions.AggressiveInlining)] + public static Faction Parse(string s, IFormatProvider provider) => Parse(s.AsSpan(), provider); + + [MethodImpl(MethodImplOptions.AggressiveInlining)] + public static bool TryParse(string s, IFormatProvider provider, out Faction result) => + TryParse(s.AsSpan(), provider, out result); + + public static Faction Parse(ReadOnlySpan s, IFormatProvider provider) + { + if (TryParse(s, provider, out var result)) + { + return result; + } + + throw new FormatException($"The input string '{s}' was not in a correct format."); + } + + public static bool TryParse(ReadOnlySpan s, IFormatProvider provider, out Faction result) { var factions = Factions; @@ -1321,13 +1341,15 @@ public abstract class Faction : IComparable { var faction = factions[i]; - if (faction.Definition.FriendlyName.InsensitiveEquals(name)) + if (s.InsensitiveEquals(faction.Definition.FriendlyName)) { - return faction; + result = faction; + return true; } } - return null; + result = null; + return false; } public static bool InSkillLoss(Mobile mob) => m_SkillLoss.ContainsKey(mob); diff --git a/Projects/UOContent/Engines/Factions/Core/Town.cs b/Projects/UOContent/Engines/Factions/Core/Town.cs index 8aa136ff6..a30dc23a6 100644 --- a/Projects/UOContent/Engines/Factions/Core/Town.cs +++ b/Projects/UOContent/Engines/Factions/Core/Town.cs @@ -1,11 +1,12 @@ using System; using System.Collections.Generic; +using System.Runtime.CompilerServices; using Server.Targeting; namespace Server.Factions; [CustomEnum(["Britain", "Magincia", "Minoc", "Moonglow", "Skara Brae", "Trinsic", "Vesper", "Yew"])] -public abstract class Town : IComparable +public abstract class Town : IComparable, ISpanParsable { public const int SilverCaptureBonus = 10000; @@ -482,7 +483,27 @@ public abstract class Town : IComparable return null; } - public static Town Parse(string name) + [MethodImpl(MethodImplOptions.AggressiveInlining)] + public static Town Parse(string s) => Parse(s, null); + + [MethodImpl(MethodImplOptions.AggressiveInlining)] + public static Town Parse(string s, IFormatProvider provider) => Parse(s.AsSpan(), provider); + + [MethodImpl(MethodImplOptions.AggressiveInlining)] + public static bool TryParse(string s, IFormatProvider provider, out Town result) => + TryParse(s.AsSpan(), provider, out result); + + public static Town Parse(ReadOnlySpan s, IFormatProvider provider) + { + if (TryParse(s, provider, out var result)) + { + return result; + } + + throw new FormatException($"The input string '{s}' was not in a correct format."); + } + + public static bool TryParse(ReadOnlySpan s, IFormatProvider provider, out Town result) { var towns = Towns; @@ -490,13 +511,15 @@ public abstract class Town : IComparable { var town = towns[i]; - if (town.Definition.FriendlyName.InsensitiveEquals(name)) + if (s.InsensitiveEquals(town.Definition.FriendlyName)) { - return town; + result = town; + return true; } } - return null; + result = null; + return false; } [Usage("GrantTownSilver ")] diff --git a/Projects/UOContent/Utilities/Types.cs b/Projects/UOContent/Utilities/Types.cs index 810ff0310..fa669d96a 100644 --- a/Projects/UOContent/Utilities/Types.cs +++ b/Projects/UOContent/Utilities/Types.cs @@ -1,4 +1,5 @@ using System; +using System.Collections.Concurrent; using System.Collections.Generic; using System.Globalization; using System.Reflection; @@ -10,7 +11,8 @@ namespace Server { public static readonly Type[] ParseStringParamTypes = { typeof(string), typeof(IFormatProvider) }; public static readonly Type[] ParseStringNumericParamTypes = { typeof(string), typeof(NumberStyles) }; - private static object[] _parseParams = { null, null }; + // Legacy RunUO signature: a static Parse(string) that predates IParsable (e.g. Faction, Town). + public static readonly Type[] ParseStringSingleParamTypes = { typeof(string) }; public static readonly Type OfByte = typeof(byte); public static readonly Type OfSByte = typeof(sbyte); @@ -75,7 +77,9 @@ namespace Server OfULong }; - private static Dictionary _isParsable; + // Thread-safe: parse metadata is read from parallel callers (e.g. the Advanced Search workers), + // not just the single-threaded command path. + private static readonly ConcurrentDictionary _isParsable = new(); [MethodImpl(MethodImplOptions.AggressiveInlining)] public static bool IsType(Type type, Type check) => check.IsAssignableFrom(type); @@ -89,25 +93,19 @@ namespace Server [MethodImpl(MethodImplOptions.AggressiveInlining)] public static bool IsText(Type t) => IsType(t, OfText); - public static bool IsParsable(Type t) - { - _isParsable ??= new(); - if (_isParsable.TryGetValue(t, out var isParsable)) + public static bool IsParsable(Type t) => + _isParsable.GetOrAdd(t, static type => { - return isParsable; - } - - foreach (var x in t.GetInterfaces()) - { - if (x.IsGenericType && x.GetGenericTypeDefinition() == typeof(IParsable<>)) + foreach (var x in type.GetInterfaces()) { - isParsable = true; - break; + if (x.IsGenericType && x.GetGenericTypeDefinition() == typeof(IParsable<>)) + { + return true; + } } - } - return _isParsable[t] = isParsable; - } + return false; + }); [MethodImpl(MethodImplOptions.AggressiveInlining)] public static bool IsDecimal(Type t) => Array.IndexOf(DecimalTypes, t) >= 0; @@ -118,18 +116,81 @@ namespace Server [MethodImpl(MethodImplOptions.AggressiveInlining)] public static bool IsEntity(Type t) => OfEntity.IsAssignableFrom(t); - private static Dictionary _parseMethods; + private static readonly ConcurrentDictionary _parseMethods = new(); + + // A static string Parse method: the modern IParsable Parse(string, IFormatProvider), or a + // legacy RunUO Parse(string). Cached per type; null if the type has neither. (Span-based Parse + // can't be reflection-invoked — a ReadOnlySpan can't be boxed into the args array — so the + // string overloads are what we bind to.) + public static MethodInfo GetParseMethod(Type t) => + _parseMethods.GetOrAdd( + t, + static type => type.GetMethod("Parse", ParseStringParamTypes) + ?? type.GetMethod("Parse", ParseStringSingleParamTypes) + ); public static object Parse(Type t, string value) { - _parseMethods ??= new(); - if (!_parseMethods.TryGetValue(t, out var method)) + var method = GetParseMethod(t); + if (method == null) { - _parseMethods[t] = method = t.GetMethod("Parse", ParseStringParamTypes); + return null; } - _parseParams[0] = value; - return method?.Invoke(null, _parseParams); + // Fresh args array per call — a shared static array would race across concurrent callers. + // Arg shape depends on which overload we bound to (IParsable 2-arg vs legacy 1-arg). + var args = method.GetParameters().Length == 2 ? new object[] { value, null } : new object[] { value }; + return method.Invoke(null, args); + } + + // Parses directly into the concrete numeric type via INumber.TryParse (the Type-dispatched + // equivalent of a generic TryParse). Returns the boxed value; false if the text doesn't fit + // the type's range/format so the caller can fall through. + private static bool TryParseNumeric(Type type, ReadOnlySpan span, NumberStyles style, out object result) + { + if (type == OfInt && int.TryParse(span, style, null, out var i)) + { + result = i; + return true; + } + if (type == OfUInt && uint.TryParse(span, style, null, out var ui)) + { + result = ui; + return true; + } + if (type == OfLong && long.TryParse(span, style, null, out var l)) + { + result = l; + return true; + } + if (type == OfULong && ulong.TryParse(span, style, null, out var ul)) + { + result = ul; + return true; + } + if (type == OfShort && short.TryParse(span, style, null, out var s)) + { + result = s; + return true; + } + if (type == OfUShort && ushort.TryParse(span, style, null, out var us)) + { + result = us; + return true; + } + if (type == OfByte && byte.TryParse(span, style, null, out var b)) + { + result = b; + return true; + } + if (type == OfSByte && sbyte.TryParse(span, style, null, out var sb)) + { + result = sb; + return true; + } + + result = null; + return false; } // Do not use this in "Parse" methods, it may cause a stack overflow @@ -201,35 +262,38 @@ namespace Server if (IsNumeric(type)) { - try + var span = value.AsSpan(); + var style = NumberStyles.Integer; + if (span.StartsWithOrdinal("0x")) { - var isHex = value.StartsWithOrdinal("0x"); - var index = isHex ? 2 : 0; - if (ulong.TryParse(value.AsSpan(index), isHex ? NumberStyles.HexNumber : NumberStyles.Integer, null, out var num)) - { - if (isEntity) - { - constructed = World.FindEntity((Serial)num); - } - else if (isSerial) - { - constructed = (Serial)num; - } - else - { - constructed = Convert.ChangeType(num, type); - } + span = span[2..]; + style = NumberStyles.HexNumber; + } + if (isEntity || isSerial) + { + // Serial/entity properties were mutated to int above; a Serial is a uint, so parse + // the full 32-bit range as ulong and resolve. + if (ulong.TryParse(span, style, null, out var num)) + { + constructed = isEntity ? World.FindEntity((Serial)num) : (Serial)num; return null; } } - catch + else if (TryParseNumeric(type, span, style, out constructed)) { - return "That is not properly formatted."; + // Parse the string directly into the target type via INumber.TryParse — no + // Convert.ChangeType, and (unlike parse-as-ulong) signed and per-type ranges are honored. + return null; } + + // On parse failure, fall through to the Parse-method / Convert.ChangeType fallbacks below. } - if (IsParsable(type)) + // IParsable (Parse(string, IFormatProvider)) or a legacy RunUO Parse(string). Gating on + // the discovered method rather than the IParsable interface keeps pre-IParsable types + // (Faction, Town, ...) parseable for backwards compatibility. + if (GetParseMethod(type) != null) { try { diff --git a/dev-docs/runuo-migration-docs/01-foundation-changes.md b/dev-docs/runuo-migration-docs/01-foundation-changes.md index 0af53c6d7..668701bea 100644 --- a/dev-docs/runuo-migration-docs/01-foundation-changes.md +++ b/dev-docs/runuo-migration-docs/01-foundation-changes.md @@ -282,6 +282,65 @@ public MyItem(Serial serial) : base(serial) { } // ModernUO — DELETE THIS CONSTRUCTOR. The source generator creates it. ``` +## 15. Static `Parse(string)` → `IParsable` / `ISpanParsable` + +RunUO predates `IParsable`/`ISpanParsable` (C# 11 / .NET 7 static-abstract interface +members), so RunUO types that convert from a string expose a bare `public static T Parse(string value)`. +**ModernUO expects any such type to implement `IParsable` (string) and, where practical, +`ISpanParsable` (span; it extends `IParsable`, so implement span and you get both).** + +This matters because the engine's string→value converter, `Server.Types.TryParse` — used by `[set`, +`[props`, spawner property assignment, the conditional-command compiler (`[where`), and Advanced +Search — binds to the `Parse(string, IFormatProvider)` signature. A type with **only** a legacy +`Parse(string)` is discovered by `Types` through a reflection fallback, but that fallback is a safety +net, not the intended path: a bare `Parse(string)` is easy to miss, doesn't participate in the +span-based fast paths, and (if it returns `null` instead of throwing) makes `[set` silently assign +`null` on bad input. Convert it. + +The `Parse` overloads throw `FormatException` on failure; `TryParse` returns `false`. Delegate the +string overloads to a span core (see `Race`, `Poison`, `Point3D` for the established pattern): + +```csharp +// RunUO +public abstract class Faction : IComparable +{ + public static Faction Parse(string name) // returns null on no-match — wrong contract, not IParsable + { + // ... linear search by name ... + return null; + } +} + +// ModernUO +public abstract class Faction : IComparable, ISpanParsable +{ + [MethodImpl(MethodImplOptions.AggressiveInlining)] + public static Faction Parse(string s) => Parse(s, null); + + [MethodImpl(MethodImplOptions.AggressiveInlining)] + public static Faction Parse(string s, IFormatProvider provider) => Parse(s.AsSpan(), provider); + + [MethodImpl(MethodImplOptions.AggressiveInlining)] + public static bool TryParse(string s, IFormatProvider provider, out Faction result) => + TryParse(s.AsSpan(), provider, out result); + + public static Faction Parse(ReadOnlySpan s, IFormatProvider provider) => + TryParse(s, provider, out var result) + ? result + : throw new FormatException($"The input string '{s}' was not in a correct format."); + + public static bool TryParse(ReadOnlySpan s, IFormatProvider provider, out Faction result) + { + // ... linear search by name using s.InsensitiveEquals(...) ... + result = null; + return false; + } +} +``` + +To find un-migrated types: search for `public static [A-Za-z0-9_<>]+ Parse\(string ` and check whether +the declaring type lists `IParsable`/`ISpanParsable`. + ## Quick Checklist When migrating any RunUO script, apply these changes in order: @@ -300,6 +359,7 @@ When migrating any RunUO script, apply these changes in order: 12. [ ] Modernize property syntax 13. [ ] Remove `Serial` constructor (handled by serialization generator) 14. [ ] Update usings +15. [ ] Convert bare static `Parse(string)` to `IParsable`/`ISpanParsable` ## See Also