fix: address SubList code review findings

This commit is contained in:
Joseph Doherty
2026-02-22 20:14:48 -05:00
parent afc419ce3f
commit bc8fee8e39

View File

@@ -6,16 +6,17 @@ namespace NATS.Server.Subscriptions;
/// interested subscribers. Subscribers can have wildcard subjects to /// interested subscribers. Subscribers can have wildcard subjects to
/// match multiple published subjects. /// match multiple published subjects.
/// </summary> /// </summary>
public sealed class SubList public sealed class SubList : IDisposable
{ {
private const int CacheMax = 1024; private const int CacheMax = 1024;
private const int CacheSweep = 256; private const int CacheSweep = 256;
private readonly ReaderWriterLockSlim _lock = new(); private readonly ReaderWriterLockSlim _lock = new();
private readonly TrieLevel _root = new(); private readonly TrieLevel _root = new();
private Dictionary<string, SubListResult>? _cache = new(); private Dictionary<string, SubListResult>? _cache = new(StringComparer.Ordinal);
private uint _count; private uint _count;
private ulong _genId;
public void Dispose() => _lock.Dispose();
public uint Count public uint Count
{ {
@@ -84,7 +85,6 @@ public sealed class SubList
} }
_count++; _count++;
_genId++;
AddToCache(subject, sub); AddToCache(subject, sub);
} }
finally finally
@@ -131,7 +131,9 @@ public sealed class SubList
var tokenStr = token.ToString(); var tokenStr = token.ToString();
pathList.Add((level, node, tokenStr, isPwc, isFwc)); pathList.Add((level, node, tokenStr, isPwc, isFwc));
level = node.Next ?? new TrieLevel(); if (node.Next == null)
return; // corrupted trie state
level = node.Next;
} }
if (node == null) return; if (node == null) return;
@@ -156,7 +158,6 @@ public sealed class SubList
if (!removed) return; if (!removed) return;
_count--; _count--;
_genId++;
RemoveFromCache(sub.Subject); RemoveFromCache(sub.Subject);
// Prune empty nodes (walk backwards) // Prune empty nodes (walk backwards)
@@ -216,9 +217,13 @@ public sealed class SubList
} }
else else
{ {
var queueSubsArr = new Subscription[queueSubs.Count][];
for (int i = 0; i < queueSubs.Count; i++)
queueSubsArr[i] = queueSubs[i].ToArray();
result = new SubListResult( result = new SubListResult(
plainSubs.ToArray(), plainSubs.ToArray(),
queueSubs.Select(q => q.ToArray()).ToArray()); queueSubsArr);
} }
if (_cache != null) if (_cache != null)
@@ -498,7 +503,7 @@ public sealed class SubList
private sealed class TrieLevel private sealed class TrieLevel
{ {
public readonly Dictionary<string, TrieNode> Nodes = new(); public readonly Dictionary<string, TrieNode> Nodes = new(StringComparer.Ordinal);
public TrieNode? Pwc; // partial wildcard (*) public TrieNode? Pwc; // partial wildcard (*)
public TrieNode? Fwc; // full wildcard (>) public TrieNode? Fwc; // full wildcard (>)
} }
@@ -507,7 +512,7 @@ public sealed class SubList
{ {
public TrieLevel? Next; public TrieLevel? Next;
public readonly HashSet<Subscription> PlainSubs = []; public readonly HashSet<Subscription> PlainSubs = [];
public readonly Dictionary<string, HashSet<Subscription>> QueueSubs = new(); public readonly Dictionary<string, HashSet<Subscription>> QueueSubs = new(StringComparer.Ordinal);
public bool IsEmpty => PlainSubs.Count == 0 && QueueSubs.Count == 0 && public bool IsEmpty => PlainSubs.Count == 0 && QueueSubs.Count == 0 &&
(Next == null || (Next.Nodes.Count == 0 && Next.Pwc == null && Next.Fwc == null)); (Next == null || (Next.Nodes.Count == 0 && Next.Pwc == null && Next.Fwc == null));