镜像站点 · 本页由第三方 GitHub 只读镜像提供,非 GitHub 官方站点,不接受任何登录或凭据输入。前往 github.com
Skip to content

Disposing the manager from IKeybindingManagerFactory.CreateManager() disposes the DI singleton, so every other KeybindingManager user then throws ObjectDisposedException #188

Closed

Description

@matt-edmondson

What's wrong

KeybindingManagerFactory.CreateManager() (Keybinding/Services/KeybindingManagerFactory.cs:28-36) first tries to resolve the manager from the container:

if (_serviceProvider.GetService(typeof(KeybindingManager)) is KeybindingManager manager)
    return manager;

AddKeybinding registers KeybindingManager as a singleton (Keybinding/Extensions/ServiceCollectionExtensions.cs:58), so CreateManager() hands out the shared container instance. Its sibling CreateManager(string dataDirectory) (:58) returns a fresh manager that the caller owns.

Two overloads of the same factory method therefore differ in ownership. Nothing in the API or the docs says which result the caller may dispose. KeybindingManager.Dispose() (Keybinding/KeybindingManager.cs:242) sets _disposed, and every public member then throws through DisposalHelper.ThrowIfDisposed.

Failure scenario

using (var m = factory.CreateManager())   // the normal pattern for a factory product that is IDisposable
{
    await m.InitializeAsync();
}
// later, elsewhere:
await injectedManager.SaveAsync();       // ObjectDisposedException
factory.CreateManager().InitializeAsync(); // ObjectDisposedException

From then on, every consumer of the injected KeybindingManager and every later CreateManager() call fails for the rest of the process. The container also disposes the instance a second time at shutdown.

Suggested fix

Choose one:

  • Have CreateManager() always return a new manager, built from the registered ICommandRegistry/IProfileManager/IKeybindingRepository. This matches CreateManager(string)'s caller-owned semantics.
  • Or document in the interface that CreateManager() may return the shared, container-owned instance and must not be disposed. Make Dispose a no-op for container-owned instances.

Acceptance criteria

  • A test resolves IKeybindingManagerFactory from a provider built with AddKeybinding, calls CreateManager(), disposes the result, then resolves KeybindingManager from the provider and calls InitializeAsync(). It must not throw.
  • The XML docs on both CreateManager overloads state who owns, and may dispose, the returned manager.

Activity

  1. matt-edmondson commented on Oct 19, 2026

    @matt-edmondson
    ContributorAuthor

    Triage

    • Category: Bug
    • Priority: High. The normal using (factory.CreateManager()) pattern disposes the DI singleton, and every later consumer then throws ObjectDisposedException for the rest of the process.
    • Area / suggested assignee: KeybindingManagerFactory.CreateManager() and DI registration in ServiceCollectionExtensions. Owner: @matt-edmondson
    • Duplicates / in progress: not a duplicate, and no open Keybinding PR.
    • Next step: prefer the first option, so that CreateManager() always returns a new caller-owned manager. That matches CreateManager(string) and removes the ownership ambiguity rather than documenting it. Add the dispose-then-resolve test and ownership notes in the XML docs.

    Generated by Claude Code

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

Metadata

Metadata

Labels

bugSomething isn't working

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions