Skip to content

Subscription precision: suppress += on a publisher that escapes by return (interprocedural — the dual of ownership transfer) #146

Description

@PhysShell

Summary

The subscription-leak detector over-reports when an event is subscribed on a publisher that is itself a freshly-created object the method's caller news and returns. The subscription is bounded by the returned publisher's lifetime (it dies with it), so no -= is needed — but the extractor can't see that, because the construct-and-return happens in the caller.

This is field-notes entry #7 ("Event subscription on a freshly-created, returned publisher") promoted to a live tracker, following the oracle run on JamesNK/Newtonsoft.Json (see docs/notes/field-notes-patterns.md).

Worked example (Newtonsoft.Json Src/Newtonsoft.Json/JsonSerializer.cs:717, commit 4f73e74)

public static JsonSerializer Create(JsonSerializerSettings? settings)
{
    JsonSerializer serializer = new JsonSerializer();
    if (settings != null) ApplySerializerSettings(serializer, settings);
    return serializer;                              // publisher escapes to the caller
}
private static void ApplySerializerSettings(JsonSerializer serializer, JsonSerializerSettings settings)
{
    if (settings.Error != null)
        serializer.Error += settings.Error;         // flagged: "+= but never -="
}

The publisher (serializer) is the returned object: the handler lives exactly as long as the serializer the caller now holds, and dies with it. Not a leak. CodeQL/Infer# have no event-subscription query, so they stay silent (correctly) — this is an own-only over-report.

Why it is not a safe local fix

Inside ApplySerializerSettings, serializer is an opaque parameter, so SubscriptionSourceKind (frontend/roslyn/OwnSharp.Extractor/Program.cs) conservatively tiers it injected and emits a warning (not a hard error — P-004 severity tiering already hedges). Blanket-suppressing a parameter publisher would lose real leaks: void Wire(EventBus bus, Sub s){ bus.X += s.OnX; } is the same syntax (param→param), but if bus is a DI singleton it's a genuine subscription leak. The two are indistinguishable within the method — proving "bounded" requires interprocedural publisher provenance.

Proposed approach

This is the dual of ownership transfer: a += on a publisher that escapes by return is as bounded as a returned IDisposable. The fix should reuse the D5 escape/summary machinery (see docs/notes/d5-ownership-transfer.md):

  • Compute, per first-party method, whether a parameter is the constructed-and-returned value of its callers (publisher provenance) — i.e. the publisher's lifetime is caller-owned.
  • When a subscription's publisher resolves to such a "returned-fresh" provenance, lower it to the bounded/local tier (silent), not injected.
  • Precision-first: any uncertainty stays injected (advisory warning), never silently dropped, and the DI-singleton real-leak case is untouched.

Current posture (acceptable interim)

The finding already surfaces only as an advisory warning under --severity warning (the oracle's setting); the default error posture is unaffected and CI does not break. So this is a recall/precision-of-warnings refinement, not a correctness blocker.

Revisit trigger

Land alongside (or after) the D5 interprocedural escape/return-skeleton work that can supply parameter→returned-publisher provenance. Until then the conservative injected warning is the honest posture.

Prior art

Checker Framework @MustCallAlias (one obligation, two handles), RLC ownership-transfer defaults; cf. issue #122 (D5.1c) for the same "defer + track an interprocedural axis" pattern.

Origin: oracle run on Newtonsoft.Json; triaged in field-notes-patterns.md #7.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions