Fixed incorrect usage of ObservableCacheEx.Switch() instead of Observable.Switch()

[JakenVeina]

...within ObservableCache<TObject, TKey>.Connect()`, which was causing completion notifications to be dropped, and hurting performance.

Fixes #1136.

@dwcullop it looks like you already correctly identified that the root issue is in ObservableCacheEx.Switch(), but that operator wasn't the intention to be used here in the first place, so it's still worth swapping to the native Observable.Switch().

[dwcullop]

Diagnosis looks right, and swapping to the native operator here is worth doing regardless of what happens to the operator itself. Two things about the diff, and then a note about ordering, because this collides with two open PRs.

Watch() still ends in .Switch()

Connect() gets the explicit Observable.Switch(...) and the comment explaining why, but Watch() keeps the fluent .Switch() a few lines down. That one is actually fine: its element type is IObservable<Change<TObject, TKey>>, and Change<TObject, TKey> is not a changeset, so it matches neither of the DynamicData overloads and already resolves to Rx's. So there is no bug there.

The problem is that the diff now has one call written explicitly and an adjacent one written fluently, with a comment on the first saying the explicit form is necessary. That reads like the second was missed. Either make both explicit, or say in the comment that Watch is unaffected because its element type is not a changeset. Otherwise someone will come along and "finish the job", or worse, assume the fluent form is safe elsewhere.

Do(static _ => { }, observer.OnCompleted) is still there

That is the other half of the problem, and it is #1140. The two argument Do has no error handler, and the gate is completed rather than faulted when the source fails: CacheUpdateObserver.OnError disposes the suspension tracker, which completes NotificationsSuspendedObservable normally. A subscriber that connected while notifications were suspended is therefore told the sequence succeeded when it actually failed. Swapping the Switch call does not change that.

Ordering

This overlaps two open PRs, both of which touch these exact lines.

#1141 fixes #1140 by dropping the Do and the Switch together:

.Where(static areNotificationsSuspended => !areNotificationsSuspended)
.Take(1)
.SelectMany(_ => CreateConnectObservable(predicate, suppressEmptyChangeSets));

Take(1) means there is only ever one inner sequence, so SelectMany behaves identically to Switch but carries the gate's terminal event through on its own. No manual observer.OnCompleted call from inside a Do, and no Switch call to resolve to the wrong overload in the first place. It is structurally immune rather than carefully correct, and it fixes the error path at the same time.

#1137 fixes the Switch operator itself, which is a separate defect: any caller doing .Switch() on a changeset stream never receives OnCompleted, not just ObservableCache.

All three are worth having, but they need an order. Simplest is to take #1141 first, since its version of these lines supersedes the change here, and then this PR reduces to the regression test, which is the part worth keeping either way. SuspendedConnectCompletesCorrectly covers the resume path more thoroughly than anything in #1141.

Last thing: both this and #1137 say they fix #1136. Whichever merges first will close it, and the operator bug should stay tracked, since it affects anyone calling .Switch() on a changeset stream. Worth splitting that into its own issue so #1137 does not end up looking orphaned.

[dwcullop]

Actually, I don't think we should be using Observable.Switch at all because it holds a lock. Not sure if there's a real deadlock risk though. Just thinking...

[dwcullop]

Nice find on the overload resolution. That ObservableCacheEx.Switch wins over Observable.Switch for this shape isn't something you'd catch reading the line, and it explains the behaviour in #1136 neatly.

I think we only need one of this and #1141 though, and #1141 is probably the one to take. It rewrites this expression to use SelectMany instead, so Switch drops out of the call site completely and there's no overload left to resolve. It also drops the Do(..., observer.OnCompleted) on the next line, which is #1140: a source failure currently reaches a deferred subscriber as a successful completion, so their error handling never runs. Taking #1141 covers both, whereas this one fixes the completion path and leaves that second problem in place.

Worth adding that #1137 repairs ObservableCacheEx.Switch itself, so #1136 is handled for other callers regardless of what happens here.

Your diagnosis is still worth keeping, so I've filed it separately as #1148. ObservableListEx.Switch has the same signature, so it's a trap on the list side too, and it seemed a shame to lose that once this expression changes.

[JakenVeina]

The problem is that the diff now has one call written explicitly and an adjacent one written fluently, with a comment on the first saying the explicit form is necessary. That reads like the second was missed. Either make both explicit, or say in the comment that Watch is unaffected because its element type is not a changeset. Otherwise someone will come along and "finish the job", or worse, assume the fluent form is safe elsewhere.

I get the point you're making, but I disagree that it's an issue worth writing extraneous code for. The fact is, 99% of the time, the ObservableCacheEx.Switch() implementation is the one that SHOULD be used. .Connect() represents a singularly-unique exception that deserves explanation. .Watch() doesn't.

It's also a moot point, because #1133 extracts the shared logic between .Connect() and .Watch() and eliminates the issue entirely.

That is the other half of the problem, and it is #1140.

If there's already another issue for this, then we'll deal with it there. Re-stating the issue here just ends up as noise.

Simplest is to take #1141 first, since its version of these lines supersedes the change here, and then this PR reduces to the regression test, which is the part worth keeping either way. SuspendedConnectCompletesCorrectly covers the resume path more thoroughly than anything in #1141.

Works for me.