Unregister event handlers in RpcTargetInfo.DisposeAsync

[Copilot]

When target objects with events are added via AddLocalRpcTarget with NotifyClientOfEvents = true, event handlers accumulate across connection lifecycles, causing a memory leak.

Root Cause

RpcTargetInfo.DisposeAsync() disposes target objects but never unregisters event handlers. While OnJsonRpcDisconnected() calls UnregisterEventHandlersFromTargetObjects(), disposal through IAsyncDisposable bypasses this, leaving handlers attached.

Changes

• src/StreamJsonRpc/Reflection/RpcTargetInfo.cs: Call UnregisterEventHandlersFromTargetObjects() in DisposeAsync() before disposing target objects
• test/StreamJsonRpc.Tests/TargetObjectEventsTests.cs: Add tests verifying handlers are cleaned up across multiple connection lifecycles and when streams close unexpectedly

The method is idempotent, so redundant calls from OnJsonRpcDisconnected are safe.

// Before: handlers leaked across multiple connections with same target
var server = new Server();
for (int i = 0; i < 10; i++) {
    var rpc = new JsonRpc(stream);
    rpc.AddLocalRpcTarget(server, new JsonRpcTargetOptions { NotifyClientOfEvents = true });
    rpc.StartListening();
    rpc.Dispose(); // Handlers not cleaned up
}
// server.ServerEvent now has 10 handlers attached

// After: handlers properly cleaned up on each disposal

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

• s0ivsblobprodwus2134.vsblob.vsassets.io
  • Triggering command: /opt/hostedtoolcache/dotnet/dotnet dotnet restore --no-dependencies /home/REDACTED/work/vs-streamjsonrpc/.codeql-scratch/dbs/csharp/working/B42A990C1810BF03B6D8FCD090EC1B53/missingpackages_workingdir --packages /home/REDACTED/work/vs-streamjsonrpc/.codeql-scratch/dbs/csharp/working/missingpackages /p:DisableImplicitNuGetFallbackFolder=true --verbosity normal --configfile /home/REDACTED/work/vs-streamjsonrpc/.codeql-scratch/dbs/csharp/working/nugetconfig/nuget.config --force (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

• Configure Actions setup steps to set up my environment, which run before the firewall is enabled
• Add the appropriate URLs or hosts to the custom allowlist in this repository's Copilot coding agent settings (admins only)

Original prompt

---

This section details on the original issue you should resolve

<issue_title>Unregister event handlers on Dispose</issue_title>
<issue_description>I am using StreamJsonRpc version 2.21.10. I attach my server implementation to a websocket and attach a server implementation using AddLocalRpcTarget with NotifyClientOfEvents = true. As a consequence, I see my event attached to by an event receiver. The RPC object is within a using block and should be disposed when awaiting its completion causes an error.

Expected behavior: If the connection drops (and the RPC object is disposed), I would expect the dispose routine to unregister the event.

Actual behavior: After the websocket connection drops (happens frequently when I take too much time debugging), I find the event still being attached, so the invocation list of the delegate starts growing, which is a resource leak.</issue_description>

<agent_instructions>Write a test that repros the described conditions to try to repro the alleged bug. Look at the code to help you speculate as to the cause if necessary to help write the test.
If you can write the test that repros it, find the cause and fix it.</agent_instructions>

Comments on the Issue (you are @copilot in this section)

• Fixes Unregister event handlers on Dispose #1377

---

💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

[AArnott]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[Copilot]

Pull request overview

This PR fixes a memory leak where event handlers accumulate across connection lifecycles when target objects are added via AddLocalRpcTarget with NotifyClientOfEvents = true. The issue occurred because RpcTargetInfo.DisposeAsync() was disposing target objects without unregistering their event handlers.

Key Changes:

• Added a call to UnregisterEventHandlersFromTargetObjects() in RpcTargetInfo.DisposeAsync() before disposing target objects
• Added comprehensive test coverage verifying handlers are cleaned up across multiple connection lifecycles and when streams close unexpectedly

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.

File	Description
src/StreamJsonRpc/Reflection/RpcTargetInfo.cs	Added call to unregister event handlers in DisposeAsync to prevent memory leak
test/StreamJsonRpc.Tests/TargetObjectEventsTests.cs	Added two new tests verifying event handlers are properly cleaned up on disposal and stream closure

[Copilot]

The serverRpc and streams.Item1 should be explicitly disposed after the test completes to prevent resource leaks. Consider adding cleanup in a finally block or using a using statement.

Suggested change

	serverRpc.StartListening();
	clientRpc.StartListening();
	// Verify handler is registered
	Assert.NotNull(sharedServer.ServerEventAccessor);
	// Simulate connection drop by closing the stream without disposing JsonRpc
	streams.Item2.Dispose();
	// Wait for the disconnection to be detected
	await serverRpc.Completion.WithCancellation(this.TimeoutToken);
	// Verify handlers are unregistered after stream closure
	Assert.Null(sharedServer.ServerEventAccessor);
	try
	{
	serverRpc.StartListening();
	clientRpc.StartListening();
	// Verify handler is registered
	Assert.NotNull(sharedServer.ServerEventAccessor);
	// Simulate connection drop by closing the stream without disposing JsonRpc
	streams.Item2.Dispose();
	// Wait for the disconnection to be detected
	await serverRpc.Completion.WithCancellation(this.TimeoutToken);
	// Verify handlers are unregistered after stream closure
	Assert.Null(sharedServer.ServerEventAccessor);
	}
	finally
	{
	clientRpc.Dispose();
	serverRpc.Dispose();
	streams.Item1.Dispose();
	streams.Item2.Dispose();
	}

[Copilot]

The call to UnregisterEventHandlersFromTargetObjects should be synchronized with SyncObject to ensure thread safety. The eventReceivers field is accessed and modified under the SyncObject lock elsewhere in the code (see AddLocalRpcTarget at line 166 and RevertAddLocalRpcTarget.Dispose at line 350), but this call occurs outside any lock. This could lead to race conditions if event handlers are being added concurrently during disposal.

Suggested change

	// Unregister event handlers first to prevent any events from being raised during disposal.
	this.UnregisterEventHandlersFromTargetObjects();
	List<object>? objectsToDispose;
	lock (this.SyncObject)
	{
	List<object>? objectsToDispose;
	lock (this.SyncObject)
	{
	// Unregister event handlers first to prevent any events from being raised during disposal.
	this.UnregisterEventHandlersFromTargetObjects();

[Copilot]

The created streams should be disposed to prevent resource leaks in the test. Consider wrapping the stream creation and rpc disposal in a using statement or explicitly disposing the streams after the rpc.Dispose() call.

Suggested change

	rpc.Dispose();
	rpc.Dispose();
	streams.Item1.Dispose();
	streams.Item2.Dispose();

[Copilot]

Local scope variable 'serverRpc' shadows TargetObjectEventsTests.serverRpc.

[Copilot]

Local scope variable 'clientRpc' shadows TargetObjectEventsTests.clientRpc.

Suggested change

	var clientRpc = new JsonRpc(streams.Item2);
	serverRpc.StartListening();
	clientRpc.StartListening();
	var localClientRpc = new JsonRpc(streams.Item2);
	serverRpc.StartListening();
	localClientRpc.StartListening();