The Fluent Handler That Explodes on Success

Part eight noted that rolling your own dispatcher costs you MediatR's pipeline behaviours — the reusable wrappers that put validation, logging and error handling around every handler. DShop.Common has exactly one attempt at such a wrapper: a fluent Handler that lets you chain Handle(...).OnSuccess(...).OnError(...).ExecuteAsync(). It is a nice little builder, it is registered in the container, it is used by nobody, and it contains a NullReferenceException that fires on the success path and spares the failure path. This is the most instructive dead code in the repository, because the bug is a single-character consequence of the await and null-conditional operators meeting in the wrong place.

The builder

Handler accumulates callbacks and runs them in a try/catch/finally:

public class Handler : IHandler
{
    private Func<Task> _handle;
    private Func<Task> _onSuccess;
    private Func<Task> _always;
    private Func<Exception, Task> _onError;
    private Func<DShopException, Task> _onCustomError;

    public Handler() => _always = () => Task.CompletedTask;

    public IHandler Handle(Func<Task> handle) { _handle = handle; return this; }
    public IHandler OnSuccess(Func<Task> onSuccess) { _onSuccess = onSuccess; return this; }
    // OnError, OnCustomError, Always ...

    public async Task ExecuteAsync()
    {
        bool isFailure = false;
        try            { await _handle(); }
        catch (DShopException customException)
        {
            isFailure = true;
            await _onCustomError?.Invoke(customException);
        }
        catch (Exception exception)
        {
            isFailure = true;
            await _onError?.Invoke(exception);
        }
        finally
        {
            if (!isFailure) { await _onSuccess?.Invoke(); }
            await _always?.Invoke();
        }
    }
}

The intent is lovely: run the work, branch to success or error, always run the cleanup. Now look at the one line in the finally that runs when everything went right: await _onSuccess?.Invoke();.

Why the happy path throws

Walk the types. _onSuccess is a Func<Task>. If nobody called .OnSuccess(...), it is null. The null-conditional operator ?. means “if _onSuccess is null, don't call Invoke, evaluate the whole expression to null instead.” So _onSuccess?.Invoke() becomes (Task)null. And then the code does await (Task)null.

await null throws a NullReferenceException. Awaiting a null task is not a no-op; the async machinery calls GetAwaiter() on the task reference, the reference is null, and you get an NRE at the await. So a handler that ran perfectly — _handle() completed, no exception, isFailure is false — walks into the finally, hits await _onSuccess?.Invoke() with an unset _onSuccess, and detonates. The success path is the dangerous one.

Now notice which delegate is safe: _always. It is the only callback initialised in the constructor — _always = () => Task.CompletedTask — so await _always?.Invoke() always awaits a real completed task. The exact fix the other delegates needed was applied to precisely one of them. The bug is inconsistent initialisation dressed up as an await gotcha.

The failure paths have the same latent trap — await _onCustomError?.Invoke(...) throws an NRE if you never set OnCustomError, which then replaces the domain exception you were trying to handle with a meaningless NullReferenceException. But the success path is the one that bites first, because “I only care about the happy case, I'll skip OnSuccess” is the most natural way to use a builder like this.

There are two one-line fixes, and both are worth knowing:

// Coalesce the null task:
if (!isFailure) await (_onSuccess?.Invoke() ?? Task.CompletedTask);

// Or initialise every delegate the way _always already is:
private Func<Task> _onSuccess = () => Task.CompletedTask;

The durable lesson outlives this class: await x?.SomethingReturningTask() is a footgun. The ?. short-circuits to a null task and the await dereferences it. Any time you await a null-conditional call, coalesce with ?? Task.CompletedTask. It is the kind of thing that only bites once and teaches a durable lesson — and the reason it never bit here is the subject of the next paragraph.

Why review misses it

The reason this survives a read-through is that the bug is invisible at the call site and only latent at the definition. await _onSuccess?.Invoke() looks defensive — the ?. reads as “safely skip if null,” which is exactly what it does for a synchronous call. The trap is that the thing being skipped-to is a Task, and the await that follows dereferences it. A reviewer's eye parses ?. as “handled the null” and moves on; the NRE is hiding one operator to the right. Making every callback non-null at construction removes the ambiguity entirely:

private Func<Task> _handle       = () => Task.CompletedTask;
private Func<Task> _onSuccess    = () => Task.CompletedTask;
private Func<Task> _always       = () => Task.CompletedTask;
private Func<Exception, Task> _onError       = _ => Task.CompletedTask;
private Func<DShopException, Task> _onCustomError = _ => Task.CompletedTask;

With that, every await _x?.Invoke() in ExecuteAsync becomes await of a real task, the ?. is redundant, and there is no path that awaits null. The class the estate shipped initialised exactly one of these five — _always — which is why _always is the only callback you can safely omit. One consistent line of initialisation, applied to one field, would have been applied to all five.

Dead, but the lesson is live

I checked: nothing in the estate injects IHandler or calls ExecuteAsync. Every service handles its own try/catch inline, and the fluent Handler sits registered in the container as a capability nobody reached for. So the NRE has never fired in anger — it is a trap laid for the first developer who tries to adopt the wrapper, defused only by disuse. To be fair, that also means it never hurt anyone, and a builder like this is a perfectly reasonable idea that MediatR's behaviours or a decorator would have generalised. The bug is not the concept; it is one uninitialised field.

The wire tap that cut the wire

If the dead handler is a latent swallow, the estate has a live one, and it is worth pairing with it. JaegerStagedMiddleware sits in the RabbitMQ pipe to trace message processing:

public override async Task InvokeAsync(IPipeContext context, CancellationToken token)
{
    using (var scope = BuildScope(messageType, correlationContext.SpanContext))
    {
        var span = scope.Span;
        try
        {
            await Next.InvokeAsync(context, token);
        }
        catch (Exception ex)
        {
            span.SetTag(Tags.Error, true);
            span.Log(ex.Message);
        }
    }
}

A tracing middleware is supposed to be a Wire Tap — it observes what flows past and lets it flow. This one catches, tags the span, logs the message, and does not rethrow. As written, an exception passing through this stage is recorded on the span and then absorbed; the wire tap has cut the wire it was tapping. The intent was surely “make sure a tracing failure doesn't break message handling,” but the catch (Exception) is broad enough to swallow the handler's failure too, leaving nothing but a red span behind. Observability code that silently changes control flow is its own hazard — the tool meant to help you see failures becomes the reason one disappears.

Two pieces of error-handling code, then, that do the opposite of their job: a wrapper that throws when nothing went wrong, and a tracer that stays quiet when something did. Both are small; both are the kind of thing a reader catches and a runtime hides.

Next, we leave error handling for service discovery — three load balancers hiding behind one configuration string, chosen at startup by a single word in a JSON file.