Refactor server iteration to ServerSource (reduce allocations, improve clarity)

[LeaFrock]

Summary

This PR refactors the existing ServerEnumerable / ServerIteratorFactory implementation into a new ServerSource type.

Motivation

Close #666

Changes

• Introduce ServerSource, a readonly struct that encapsulates server enumeration logic.
• Remove ServerIteratorFactory and ServerEnumerable.

Checklist

• Code compiles without warnings (TreatWarningsAsErrors is enabled)
• Tests pass locally (dotnet test)
• New code has test coverage
• No breaking changes to public API (or documented in PR description)

[imperugo]

Thanks @LeaFrock — the refactor itself is sound, and I've verified it merges cleanly alongside #664 with the full suite green (1754 + 26 tests on net10.0). But there's a blocker on when it can ship, plus a few smaller points.

The blocker: this is a breaking change

The PR removes two public types from the public namespace StackExchange.Redis.Extensions.Core.ServerIteration, both with XML docs:

public static class ServerIteratorFactory   // removed
public class ServerEnumerable               // removed

The checklist has "No breaking changes to public API" ticked, but anyone calling ServerIteratorFactory.GetServers(...) or constructing ServerEnumerable stops compiling. Neither type appears in our documentation, so real-world usage is probably near zero — but it's still a public API removal, and that means a major version.

Two ways forward, your call:

A — ship in v13.5 (soon), same pattern as #662. Keep both public types as thin [Obsolete] wrappers delegating to ServerSource, with DiagnosticId set so consumers can suppress this specific deprecation rather than blanket-disabling CS0618. They get a compile-time warning pointing at the replacement, and we delete them in v14. Note our build runs TreatWarningsAsErrors, so any internal call-site of the obsolete types has to move to ServerSource in the same PR.

B — retarget to v14. Clean removal, no wrappers, no deprecation cycle — but it sits parked until the v14 milestone opens, which isn't scheduled yet.

I'd go with A if you want this out with the next release; B only if you'd rather not carry the wrappers.

Smaller points

Missing copyright header. ServerSource.cs starts straight at using System.Collections.Generic;. Both files it replaces carried the standard // Copyright (c) Ugo Lattanzi... header.

Field naming. _endPoints uses an underscore prefix; the codebase uses bare camelCase throughout (connectionPoolManager, serverEnumerationStrategy, keyPrefix, maxValueLength).

No tests, despite the checklist. No test files are touched here, and there's no coverage of server iteration either before or after — so it's not a regression. But the Single path was rewritten from scratch, and one test distinguishing Single from All would be cheap insurance.

Eager/lazy asymmetry. GetServers is now eager for Single (it calls multiplexer.GetServer immediately) while All stays lazy via yield return. Previously both were lazy. There's one call-site and it enumerates immediately, so no impact today — but it's a trap for a future caller who assumes deferred execution, and it also moves where an exception from GetServer surfaces. Worth either making both eager or documenting the asymmetry on the method.

A note on the perf claim. Caching GetEndPoints() only pays off if the same ServerSource is enumerated more than once, and at the single call-site it's constructed fresh inside SearchKeysAsync — so GetEndPoints() runs exactly once either way. The real win here is dropping the TakeFirst iterator and the ServerEnumerable object, which is genuine, just smaller than #666 describes. Worth correcting the issue so we don't over-credit it in the release notes.

Off-topic change. pattern = $"{keyPrefix}{pattern}" → keyPrefix + pattern is fine (arguably better), but it's unrelated to server iteration and would be easier to review on its own.

Let me know which option you'd prefer for the breaking change and I'll set the milestone accordingly.

[imperugo]

Decision from my side: let's go with option A, the [Obsolete] wrappers, so this ships together with #664 and #665 in v13.5 rather than waiting for a v14 that isn't scheduled yet.

I've moved #666 to the v13.5 milestone accordingly.

Concretely, what that means for this PR:

Keep both public types, delegating to ServerSource. ServerIteratorFactory.GetServers(multiplexer, strategy) becomes a one-liner over new ServerSource(multiplexer).GetServers(strategy). ServerEnumerable is the slightly fiddlier one, since its constructor takes targetRole and unreachableServerAction separately rather than a whole strategy — but ServerEnumerationStrategy has settable properties and sensible defaults, so its GetEnumerator can just build one with Mode = All and hand it to ServerSource. Behaviour stays identical: ServerEnumerable always had All semantics.

Mark both [Obsolete] with a DiagnosticId, e.g. SRE0002 (SRE0001 is reserved for the AddAllAsync deprecation in #662), pointing at ServerSource and stating they're removed in v14. The DiagnosticId matters: without it consumers have to disable CS0618 project-wide to silence us, which also hides deprecations coming from every other library they use.

Watch out for TreatWarningsAsErrors. The moment the attribute goes on, any internal call-site of the obsolete types fails the build — so SearchKeysAsync needs to be on ServerSource in the same commit. That's already the case in this PR, so you should be fine, but worth re-checking after you add the wrappers.

I'll open the v14 follow-up for the actual removal once this lands, mirroring what we did for #662 and #663.

No pressure to take this on if you'd rather not carry the wrappers — say the word and I'll push the change onto the branch myself, same as I did for #664. Either way the other points from my previous comment still stand (copyright header, _endPoints naming, the eager/lazy asymmetry, and a test for the Single path).

[LeaFrock]

Eager/lazy asymmetry....

Great catch. I did ignore the diff which actually brings a break change. Let's keep both ways lazy in order to bring lowest risks.

Mark both [Obsolete] with a DiagnosticId

The netstandard 2.1 does not support DiagnosticId in ObsoleteAttribute. I don't know how to deal with it currently.

A note on the perf claim...

Issue updated.

Off-topic change. .. but it's unrelated to server iteration and would be easier to review on its own

In fact it's suggested by the default VS intelligence. Sometimes this kind of code style fix or optimization is too small to create a separated issue/PR... However, if you indeed mind this, I'll do as you wish.

No tests....

Well, you may notice that ServerSource is internal rather than public. I actually think both ServerIteratorFactory/ServerEnumerable should not be public at all—they are much better suited as implementation details tailored to this library’s specific needs, rather than APIs exposed to consumers.

I propose we simply remove them in 14.0 and avoid exposing the new ServerSource struct as well. If we receive concrete feedback from downstream consumers later, we can always reintroduce them at that point—this approach avoids any future breaking changes.

On the testing side, given that multiple tests already cover SearchKeysAsync, introducing additional tests solely for ServerSource(which is internal) feels unnecessary in my own opinion.

[imperugo]

Thanks for going through these one by one.

Off-topic change — you're right, I was being pedantic. If VS suggests it and it's a one-liner, folding it in is fine. Leave it.

Eager/lazy — good, keeping both lazy is the right call. It also keeps the exception timing from GetServer where it is today, which is one less thing for anyone to notice.

DiagnosticId on netstandard2.1

You're right, and I checked rather than assumed: ObsoleteAttribute.DiagnosticId arrived in .NET 5, and on netstandard2.1 the compiler gives error CS0246: The type or namespace name 'DiagnosticId' could not be found. Good catch.

It's one #if away, though. I compiled this against both targets:

#if NET5_0_OR_GREATER
[Obsolete("Use ServerSource instead. This type is removed in v14.", DiagnosticId = "SRE0002")]
#else
[Obsolete("Use ServerSource instead. This type is removed in v14.")]
#endif

Verified output: warning SRE0002 on the net8.0 target, warning CS0618 on netstandard2.1. Both compile, both warn.

And the degradation is narrower than it looks. We multi-target netstandard2.1;net8.0;net9.0;net10.0, so anyone on .NET 8/9/10 resolves the net8.0 asset and gets the suppressible id. Only .NET Core 3.x, Mono, Xamarin and Unity consumers land on the netstandard2.1 asset — and they still get a warning, just the generic one.

Tests — the Single path genuinely has zero coverage

I take the point that ServerSource is internal and that SearchKeysAsync is exercised by plenty of tests. But I went and checked what those tests actually configure:

// tests/.../Helpers/RedisConfigurationForTest.cs:26-31
ServerEnumerationStrategy = new()
{
    Mode = ServerEnumerationStrategy.ModeOptions.All,   // <-- always All
    ...
}

Every test pins Mode = All. So the Single branch is covered by nothing at all, before or after this PR — and Single is precisely the path this PR reworks hardest: TakeFirst deleted, execution made eager, and now moving back to lazy. That's three rewrites of the one branch with no test underneath it.

Internal visibility isn't the obstacle here; the test drives it from the outside by setting Mode = Single and asserting a single server is touched. One test, and the branch stops being a blind spot.

On removing them outright in 14.0

On the architecture I agree with you completely: ServerIteratorFactory and ServerEnumerable should never have been public. They're implementation details shaped around this library's needs, and keeping ServerSource internal is the right destination.

But that's an argument about where we end up, not about how we get there. The deprecation cycle isn't there to litigate whether those types deserve to be public — it's there to decide whether someone using them today gets a release of warning or a compile error with no notice.

There's also a consistency cost. We just agreed in #662 to deprecate AddAllAsync(Tuple<string, T>[] ...) before removing it, and opened #663 for the removal in v14. If in the same major we deprecate a bulk-write API but silently delete two documented public types, our deprecation policy reads as arbitrary to anyone tracking the project.

And the practical consequence: without the wrappers this PR can't ship in v13.5. It sits until v14 opens, which isn't scheduled. #664 and #665 are ready now, and I'd rather this went out with them.

So I'd like to hold the line on the wrappers. The honest cost is about thirty lines of throwaway code carried for a single release — I don't think that's a bad trade for giving consumers notice, and it keeps us consistent with what we just told people in #662.

Offer

You've already done the substantive work here, and the wrappers plus a Single-mode test are chore work. If you'd rather not carry them, say so and I'll push both onto this branch myself — same as I did on #664. No hard feelings either way, and it doesn't change the credit for the refactor.

Remaining small stuff whenever you touch it next: the copyright header on ServerSource.cs, and _endPoints → endPoints to match the codebase (connectionPoolManager, keyPrefix, and friends all go bare).

[LeaFrock]

without the wrappers this PR can't ship in v13.5...

Got it. I've changed both of them into a slim wrapper of ServerSource.

The message of ObsoleteAttribute cannot include 'Use ServerSource instead' as the ServerSource pretends to be internal.

From a more conservative standpoint, ServerIteratorFactory could even remain public as-is—it’s currently just a thin static wapper of ServerSource, so the call is yours. As for ServerEnumerable, although it is technically public, I’m skeptical that anyone is actually consuming it in practice. I’d suggest simply marking it as obsolete and communicating its upcoming removal to consumers.

If you'd rather not carry them, say so and I'll push both onto this branch myself

Thank you very much for the positive feedback on my PR.

On the unit test side, I’m not yet sure how to add coverage in a way that’s meaningful without introducing unnecessary churn, so I’d prefer to leave that part to you.
Coincidentally, issue #668 is related to tests—you can address both together if that makes sense.

[imperugo]

This looks good — all four points addressed, and folding the filter into a single EnumerateCore is cleaner than the two duplicated loops I was expecting. I merged it locally against current master (which has since gained #665 and a .gitattributes normalization): no conflicts, clean build on all TFMs with TreatWarningsAsErrors, 1754 + 26 tests green.

Your point about the Obsolete message is one I'd missed — pointing consumers at an internal type would have been useless to them, and "Fire an issue if you require similar functionality" is the honest version. Good.

On ServerIteratorFactory staying public and non-obsolete: I'd keep both deprecated as you've done. If they're both implementation details, deprecating one and not the other makes the policy look arbitrary to anyone reading it from outside.

One follow-up on Take(1)

Small thing, and it's the one item from #666 that didn't quite land: the issue said "Eliminate the TakeFirst iterator entirely", but EnumerateCore(strategy).Take(1) still allocates two iterators on the Single path — the EnumerateCore state machine plus LINQ's. In practice Take(1) has taken TakeFirst's place.

To be clear about the stakes: the allocation itself is noise. SearchKeysAsync issues a SCAN across the keyspace, so one iterator object is nothing next to the network round-trips. The argument is simplicity, not speed — the version below is one method instead of two, drops the System.Linq import from the file, and does what the issue said it would:

public IEnumerable<IServer> GetServers(ServerEnumerationStrategy strategy)
{
    foreach (var endPoint in endPoints)
    {
        var server = multiplexer.GetServer(endPoint);

        if (strategy.TargetRole == TargetRoleOptions.PreferSlave)
        {
            if (!server.IsReplica)
                continue;
        }

        if (strategy.UnreachableServerAction == UnreachableServerActionOptions.IgnoreIfOtherAvailable)
        {
            if (!server.IsConnected || !server.Features.Scan)
                continue;
        }

        yield return server;

        if (strategy.Mode == ModeOptions.Single)
            yield break;
    }
}

Single becomes a yield break after the first match instead of an outer operator. Still fully lazy for both modes, no duplicated filter, one iterator. ServerSource.cs goes from 50 lines to 47.

I built and ran this locally — behaviourally identical to yours.

Tests

Happy to take these on, as you suggested. Here's what I've already written and run against both implementations (yours and the one above — all three pass on either, which is how I confirmed they're equivalent):

[Fact]
public void GetServers_SingleMode_ReturnsOnlyFirstServer()
{
    var (mux, servers) = BuildMultiplexer(3);
    var strategy = new ServerEnumerationStrategy { Mode = ServerEnumerationStrategy.ModeOptions.Single };

    var result = new ServerSource(mux).GetServers(strategy).ToList();

    Assert.Single(result);
    Assert.Same(servers[0], result[0]);
}

[Fact]
public void GetServers_AllMode_ReturnsEveryServer() { /* 3 endpoints in, 3 servers out */ }

[Fact]
public void GetServers_IsLazy_DoesNotTouchMultiplexerUntilEnumerated()
{
    // for each mode: GetServers(...) must not call GetServer until the result is enumerated
    mux.DidNotReceive().GetServer(Arg.Any<EndPoint>());
    _ = lazy.ToList();
    mux.Received().GetServer(Arg.Any<EndPoint>());
}

IConnectionMultiplexer mocks cleanly with NSubstitute, so no Redis instance is needed and it runs in ~50ms. InternalsVisibleTo is already configured, so testing the internal struct directly works — worth noting since that was the sticking point. The laziness test is the one I care about most: it's what would have caught the eager/lazy regression automatically instead of by review.

Want me to push both the ServerSource simplification and the tests onto this branch, or would you rather take the simplification yourself and leave me the tests? Either is fine — say the word and I'll do it today so this can go out with #664 in v13.5.

[LeaFrock]

Interesting...

BenchmarkDotNet v0.15.8, Windows 11 (10.0.26200.8973/25H2/2025Update/HudsonValley2)
Intel Core i7-8700 CPU 3.20GHz (Max: 3.19GHz) (Coffee Lake), 1 CPU, 12 logical and 6 physical cores
.NET SDK 10.0.302
  [Host]    : .NET 10.0.10 (10.0.10, 10.0.1026.32716), X64 RyuJIT x86-64-v3
  .NET 10.0 : .NET 10.0.10 (10.0.10, 10.0.1026.32716), X64 RyuJIT x86-64-v3
  .NET 8.0  : .NET 8.0.29 (8.0.29, 8.0.2926.32403), X64 RyuJIT x86-64-v3

Method	Job	Runtime	mode	Mean	Error	StdDev	Gen0	Allocated
Run1	.NET 10.0	.NET 10.0	0	51.10 ns	0.150 ns	0.126 ns	0.0051	32 B
Run2	.NET 10.0	.NET 10.0	0	55.84 ns	0.175 ns	0.163 ns	0.0063	40 B
Run1	.NET 8.0	.NET 8.0	0	66.25 ns	1.143 ns	1.069 ns	0.0050	32 B
Run2	.NET 8.0	.NET 8.0	0	65.16 ns	0.421 ns	0.352 ns	0.0063	40 B
Run1	.NET 10.0	.NET 10.0	1	22.34 ns	0.447 ns	0.396 ns	0.0140	88 B
Run2	.NET 10.0	.NET 10.0	1	10.65 ns	0.241 ns	0.470 ns	0.0064	40 B
Run1	.NET 8.0	.NET 8.0	1	24.90 ns	0.521 ns	0.730 ns	0.0140	88 B
Run2	.NET 8.0	.NET 8.0	1	11.26 ns	0.209 ns	0.185 ns	0.0064	40 B

using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Jobs;

namespace BenchmarkTest;

[SimpleJob(runtimeMoniker: RuntimeMoniker.Net80)]
[SimpleJob(runtimeMoniker: RuntimeMoniker.Net10_0)]
[MemoryDiagnoser]
public class YieldTest
{
    [Benchmark]
    [Arguments(0)]
    [Arguments(1)]
    public int Run1(int mode)
    {
        var e = mode == 1
            ? EnumerateNumbers().Take(1)
            : EnumerateNumbers();
        var n = 0;
        foreach (var i in e)
        {
            n |= i;
        }
        return n;
    }

    [Benchmark]
    [Arguments(0)]
    [Arguments(1)]
    public int Run2(int mode)
    {
        var n = 0;
        foreach (var i in EnumerateNumbers(mode))
        {
            n |= i;
        }
        return n;
    }


    private IEnumerable<int> EnumerateNumbers()
    {
        for (int i = 0; i < 30; i++)
        {
            yield return 1 << i;
        }
    }

    private IEnumerable<int> EnumerateNumbers(int mode)
    {
        for (int i = 0; i < 30; i++)
        {
            yield return 1 << i;
            if (mode == 1)
            {
                yield break;
            }
        }
    }
}

[imperugo]

Thanks for actually measuring it instead of us trading opinions — that settles it, and not in my favour.

Reading your numbers:

mode		allocated	mean (.NET 10)
All (0)	Take(1) variant	32 B	51.10 ns
All (0)	yield break variant	40 B	55.84 ns
Single (1)	Take(1) variant	88 B	22.34 ns
Single (1)	yield break variant	40 B	10.65 ns

They win in opposite directions. Mine halves both time and allocation on Single, because it drops the second iterator — that part I had right. But it costs 8 B and a per-item branch on All, since the mode field widens the state machine and the check runs on every iteration. And All is the default mode, so your version optimises the path that actually gets taken.

That's enough for me. Keep yours as it is.

Two honest caveats so the numbers don't get over-read in either direction. Your harness enumerates 30 items; endPoints here is 1 for a standalone server and a handful for a cluster, so the per-item branch that penalises my version barely gets to run — but by the same token the 48 B I save on Single is just as invisible. And there's exactly one call-site, SearchKeysAsync, which issues a SCAN across the keyspace. Both variants are rounding errors next to that.

Which means my original argument was the weak one anyway. I pitched it on simplicity rather than speed, and one method versus two is a thin case — thin enough that "measured better on the default path" beats it outright. Your version also reads more plainly than a yield break tucked at the bottom of the loop, which I'll admit is slightly subtle control flow.

So: nothing more to change here from my side. I'll push the three tests onto this branch — Single returns one server, All returns all of them, and neither touches the multiplexer before enumeration — and then merge it alongside #664 for v13.5. The laziness test is the one worth having: it's what would have caught the eager/lazy regression on its own rather than in review.

Nice work on this one, and thanks for the pushback on the deprecation shape earlier — the Obsolete message pointing at an internal type would have shipped as a real papercut.

[imperugo]

Tests pushed to this branch — tests/StackExchange.Redis.Extensions.Core.Tests/ServerSourceTests.cs, six of them, no Redis instance needed (IConnectionMultiplexer mocks cleanly with NSubstitute, ~50 ms for the lot).

• Single returns only the first matching server
• All returns every server
• PreferSlave skips primaries
• IgnoreIfOtherAvailable skips disconnected servers
• GetServers is lazy in both modes — the multiplexer is not touched until the result is enumerated (a [Theory] over the two modes)

The laziness one is the reason this was worth doing, and I checked it actually bites rather than just describing current behaviour: I temporarily restored the earlier eager Single implementation and it failed, then restored yours and it passed. So the deferred-execution contract you and I agreed on is now enforced by the suite instead of by review.

One gotcha worth knowing if you extend these: IServer mocks need Features stubbed to a Redis version >= 2.8, otherwise Features.Scan comes back false and the IgnoreIfOtherAvailable filter quietly discards every server. Cost me a red test before I spotted it, so there's a comment in the helper.

Full suite locally: 1760 + 26 green on net10.0 (1754 before, +6 here), clean build on all TFMs with TreatWarningsAsErrors.

I touched nothing outside the new test file — ServerSource.cs is exactly as you left it.

Not merging yet; I'll wait in case you want to look over the tests first. Once you're happy this goes in with #664 for v13.5.

[LeaFrock]

I’m leaning toward the Take-based approach for two additional, less obvious reasons:

First, LINQ APIs are far more familiar to other developers reading the code, which reduces cognitive friction—this is also why I found the previous TakeFirst implementation rather weird.

Second, even though the Single mode currently incurs slightly more allocations, .NET’s ongoing improvements to PGO, escape analysis, and stack allocation mean that idiomatic LINQ usage is more likely to benefit from future runtime optimizations than a hand-rolled, scenario-specific implementation.

The unit tests LGTM—much appreciated. Also, I can’t wait to dive into the issue #669 —trying to squeeze it in while I’ve got some free time.

[imperugo]

Agreed — and we're already aligned there, Take stays. Both your reasons hold up, and the second one is the better argument of the two: idiomatic LINQ sits on the path the runtime team keeps optimising, so escape analysis and stack allocation land on it for free over time, while a hand-rolled iterator is frozen at whatever I hand-wrote today. That outlives any 8-byte delta we measured. Your point about TakeFirst reading as odd is fair too — bespoke control flow costs every future reader a moment, and that's a real recurring price against a one-off allocation.

Thanks for looking over the tests. Nothing further from me on this PR — it goes in with #664 for v13.5 as soon as I get the go-ahead on merging.

On #669: the plan is sound and the sequencing is right — after #664 lands, SpanExtensions.cs is down to EnumerateLines alone, so folding that into InfoDetail empties the file. I've left a note on the issue about one part of it that needs a different milestone.

[imperugo]

Merging. Verified locally on top of #664 first: clean three-way merge (both PRs touched RedisDatabase.cs), clean build on all TFMs with TreatWarningsAsErrors, and 1760 + 26 tests green on net10.0.

Thanks @LeaFrock — good back-and-forth on this one. The benchmark settled the Take question properly, and your catch on the Obsolete message pointing at an internal type saved a real papercut.

Ships in v13.5. I'll open the v14 follow-up for removing the deprecated ServerIteratorFactory / ServerEnumerable, mirroring #663.