Skip to content

Conversation

@hannesrudolph
Copy link
Collaborator

@hannesrudolph hannesrudolph commented Apr 5, 2025

Problem:
Closing auxiliary windows (e.g., from "open in editor") could prematurely dispose the shared singleton McpHub instance, shutting down MCP servers unexpectedly while other panels (like the sidebar) might still be active.

Solution:
Implemented reference counting directly within the McpHub class to manage its own lifecycle based on active clients (ClineProvider instances).

  • Added refCount, registerClient(), and unregisterClient() methods to McpHub.
  • McpHub now disposes itself only when the last registered client unregisters (refCount reaches 0).
  • ClineProvider instances now call mcpHub.registerClient() upon initialization and mcpHub.unregisterClient() upon disposal.
  • Removed the direct mcpHub.dispose() call from ClineProvider.dispose.

Benefit:
This ensures the shared McpHub instance remains active as long as at least one ClineProvider instance is using it. Cleanup now correctly occurs only when the last provider is closed or during full extension deactivation. This centralizes the resource's lifecycle logic within the resource class itself.

Files Changed:

  • src/services/mcp/McpHub.ts
  • src/core/webview/ClineProvider.ts

Important

Implements reference counting in McpHub to manage its lifecycle based on active ClineProvider instances, preventing premature disposal.

  • Behavior:
    • Implements reference counting in McpHub to manage lifecycle based on active ClineProvider instances.
    • McpHub disposes itself only when refCount reaches 0.
    • ClineProvider calls registerClient() on initialization and unregisterClient() on disposal.
    • Removes direct dispose() call from ClineProvider.dispose().
  • Methods:
    • Adds registerClient() and unregisterClient() to McpHub.
  • Files:
    • Changes in McpHub.ts and ClineProvider.ts.

This description was created by Ellipsis for 0c51f38. It will automatically update as commits are pushed.

Problem:
Closing auxiliary windows (e.g., from "open in editor") could prematurely dispose the shared singleton McpHub instance, shutting down MCP servers unexpectedly while other panels (like the sidebar) might still be active.

Solution:
Implemented reference counting directly within the McpHub class to manage its own lifecycle based on active clients (ClineProvider instances).
- Added `refCount`, `registerClient()`, and `unregisterClient()` methods to McpHub.
- McpHub now disposes itself only when the last registered client unregisters (`refCount` reaches 0).
- ClineProvider instances now call `mcpHub.registerClient()` upon initialization and `mcpHub.unregisterClient()` upon disposal.
- Removed the direct `mcpHub.dispose()` call from ClineProvider.dispose.

Benefit:
This ensures the shared McpHub instance remains active as long as at least one ClineProvider instance is using it. Cleanup now correctly occurs only when the last provider is closed or during full extension deactivation. This centralizes the resource's lifecycle logic within the resource class itself.

Files Changed:
- src/services/mcp/McpHub.ts
- src/core/webview/ClineProvider.ts
@changeset-bot
Copy link

changeset-bot bot commented Apr 5, 2025

⚠️ No Changeset found

Latest commit: 0c51f38

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@dosubot dosubot bot added size:M This PR changes 30-99 lines, ignoring generated files. enhancement New feature or request labels Apr 5, 2025
* If the count reaches zero, disposes the hub.
*/
public async unregisterClient(): Promise<void> {
this.refCount--
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In unregisterClient(), consider adding a safeguard to prevent the reference count from dropping below zero before proceeding to dispose. This helps avoid potential underflow if unregisterClient() is called extra times.

Suggested change
this.refCount--
if (this.refCount > 0) this.refCount--

Copy link
Collaborator

@mrubens mrubens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Apr 5, 2025
@mrubens mrubens merged commit 3f6e07a into main Apr 5, 2025
21 checks passed
@mrubens mrubens deleted the mcp-crash-fix branch April 5, 2025 03:14
@github-project-automation github-project-automation bot moved this from New to Done in Roo Code Roadmap Apr 5, 2025
@hannesrudolph hannesrudolph mentioned this pull request Apr 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request lgtm This PR has been approved by a maintainer size:M This PR changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants