fix: Harden Advanced Search: crash-safety, autosave, correct results & worker fixes (#2543)
## 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<T>.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<T>` removed.
- `ConcurrentDictionary` for the reflection cache is intentional — these workers are genuinely parallel.
This commit is contained in:
Kamron Batman
GitHub
parent
858c1d18bc
commit
1e97ed50f6
13 changed files with 842 additions and 209 deletions
|
|
@ -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));
|
||||
}
|
||||
}
|
||||
|
|
@ -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);
|
||||
}
|
||||
}
|
||||
|
|
@ -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<TimeSpan> 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", "="));
|
||||
}
|
||||
}
|
||||
|
|
@ -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<AdvancedSearchResult>();
|
||||
var ignore = new ConcurrentQueue<IEntity>();
|
||||
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<AdvancedSearchResult>();
|
||||
var ignore = new ConcurrentQueue<IEntity>();
|
||||
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);
|
||||
}
|
||||
}
|
||||
|
|
@ -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<T>.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);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue