Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions src/StreamJsonRpc/Reflection/RpcTargetInfo.cs
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,9 @@ internal RpcTargetInfo(JsonRpc jsonRpc)

public async ValueTask DisposeAsync()
{
// Unregister event handlers first to prevent any events from being raised during disposal.
this.UnregisterEventHandlersFromTargetObjects();

List<object>? objectsToDispose;
lock (this.SyncObject)
{
Comment on lines +45 to 50
Copy link

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

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

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 uses AI. Check for mistakes.
Expand Down
74 changes: 74 additions & 0 deletions test/StreamJsonRpc.Tests/TargetObjectEventsTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,80 @@ public void GenericServerEventSubscriptionLifetime()
Assert.Null(this.server.ServerEventWithCustomArgsAccessor);
}

/// <summary>
/// Verifies that event handlers are unregistered when multiple connections are established and disposed.
/// This tests for a memory leak where event handlers would accumulate if not properly unregistered on disposal.
/// </summary>
[Fact]
public void EventHandlersUnregisteredOnMultipleConnectionDisposals()
{
// Use a shared server object across multiple connections to simulate the real-world scenario
var sharedServer = new Server();

// Verify no handlers initially
Assert.Null(sharedServer.ServerEventAccessor);
Assert.Null(sharedServer.ServerEventWithCustomArgsAccessor);

// Create and dispose multiple connections
for (int i = 0; i < 3; i++)
{
var streams = FullDuplexStream.CreateStreams();
var rpc = this.CreateJsonRpcWithTargetObject(streams.Item1, sharedServer);
rpc.StartListening();

// Verify handler is registered
Assert.NotNull(sharedServer.ServerEventAccessor);
Assert.NotNull(sharedServer.ServerEventWithCustomArgsAccessor);

// Count the number of handlers attached
int serverEventHandlerCount = sharedServer.ServerEventAccessor?.GetInvocationList().Length ?? 0;
int customArgsEventHandlerCount = sharedServer.ServerEventWithCustomArgsAccessor?.GetInvocationList().Length ?? 0;

// Should only have one handler per event, not accumulating
Assert.Equal(1, serverEventHandlerCount);
Assert.Equal(1, customArgsEventHandlerCount);

// Dispose the connection
rpc.Dispose();
Copy link

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

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

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 uses AI. Check for mistakes.

// Verify handlers are unregistered after disposal
Assert.Null(sharedServer.ServerEventAccessor);
Assert.Null(sharedServer.ServerEventWithCustomArgsAccessor);
}
}

/// <summary>
/// Verifies that event handlers are unregistered when the stream is closed without explicit disposal.
/// This simulates the scenario where a websocket connection drops unexpectedly.
/// </summary>
[Fact]
public async Task EventHandlersUnregisteredWhenStreamClosesUnexpectedly()
{
var sharedServer = new Server();

// Verify no handlers initially
Assert.Null(sharedServer.ServerEventAccessor);

var streams = FullDuplexStream.CreateStreams();
var serverRpc = this.CreateJsonRpcWithTargetObject(streams.Item1, sharedServer);
Copy link

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

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

Local scope variable 'serverRpc' shadows TargetObjectEventsTests.serverRpc.

Copilot uses AI. Check for mistakes.
var clientRpc = new JsonRpc(streams.Item2);

serverRpc.StartListening();
clientRpc.StartListening();
Comment on lines +209 to +212
Copy link

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

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

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();

Copilot uses AI. Check for mistakes.

// 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);
Comment on lines +211 to +224
Copy link

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

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

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 uses AI. Check for mistakes.
}

/// <summary>Ensures that JsonRpc only adds one event handler to target objects where events are declared multiple times in an type hierarchy.</summary>
/// <remarks>This is a regression test for <see href="https://github.com/microsoft/vs-streamjsonrpc/issues/481">this bug</see>.</remarks>
[Fact]
Expand Down
Loading