Skip to content

architecture: Program.cs DI wiring uses a global mutable closure variable for BgpServer (order-dependent, NRE-prone) #231

Description

@ruhex

What is wrong

BgpServer is registered with a side-effect-ful AddHostedService factory that writes a captured local variable, and ISessionManager then reads that variable from a separate factory:

// BGPLite/Program.cs:156-176
BgpServer? bgpServer = null;

builder.Services.AddHostedService(sp =>
{
    var store = sp.GetRequiredService<PeerStore>();
    var prefixService = sp.GetRequiredService<IPrefixService>();
    bgpServer = new BgpServer(                       // ← side effect on a captured local
        sp.GetRequiredService<AppConfig>(),
        ...
        (ip, asn) => store.UpsertPeer(ip, asn),
        store, prefixService,
        communityResolver: sp.GetRequiredService<ICommunityResolver>());
    return bgpServer;
});
builder.Services.AddSingleton<ISessionManager>(sp => bgpServer!);   // ← reads the captured local

This wiring is order-dependent and fragile:

  • The AddHostedService factory only runs when the hosted service starts (lazily, on StartAsync).
  • The ISessionManager factory runs when any consumer resolves ISessionManager — which can happen earlier (e.g. PrefixAutoRefreshService and PrefixSourceService both inject ISessionManager and may resolve it during their own construction / first use).
  • If ISessionManager resolves before the hosted-service factory has run, bgpServer is still null and bgpServer! throws NullReferenceException at runtime — hidden behind the null-forgiving !.

In the current configuration it happens to work because the hosted service starts before the consumers fire their first ISessionManager call, but this is incidental. Any change to registration order, an eager-singleton consumer, or a startup hook that resolves ISessionManager early will turn this into an NRE.

It is also inconsistent with the pattern already used in the same file for ManagementApi (Program.cs:182-183):

builder.Services.AddSingleton<ManagementApi>();
builder.Services.AddHostedService(sp => sp.GetRequiredService<ManagementApi>());

— here the singleton is the source of truth and the hosted service resolves it. The same pattern should be used for BgpServer.

Fix

Register BgpServer as a singleton and have both AddHostedService and ISessionManager resolve it from the container — no captured local:

builder.Services.AddSingleton(sp =>
{
    var store = sp.GetRequiredService<PeerStore>();
    var prefixService = sp.GetRequiredService<IPrefixService>();
    return new BgpServer(
        sp.GetRequiredService<AppConfig>(),
        ...
        (ip, asn) => store.UpsertPeer(ip, asn),
        store, prefixService,
        communityResolver: sp.GetRequiredService<ICommunityResolver>());
});
builder.Services.AddHostedService(sp => sp.GetRequiredService<BgpServer>());
builder.Services.AddSingleton<ISessionManager>(sp => sp.GetRequiredService<BgpServer>());

Now there is exactly one BgpServer instance, owned by the DI container, and every consumer resolves the same instance regardless of order.

Acceptance

  • No captured-local / mutable-field indirection between AddHostedService and ISessionManager.
  • ISessionManager resolves to a non-null instance regardless of resolution order.
  • Startup/shutdown ordering unchanged (hosted-service lifecycle preserved).
  • dotnet test startup tests green.

Refs: #93 / #88 / #98 (architecture-review cluster — this is the DI-wiring instance of the same structural debt).

Metadata

Metadata

Assignees

No one assigned

    Labels

    architectureStructural / architectural improvements

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions