Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Ensure that when EnC finishes that it disposes the OOP connection it is holding onto #73465

Closed
wants to merge 6 commits into from
Closed

Ensure that when EnC finishes that it disposes the OOP connection it is holding onto #73465

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
5ad975f
Ensure that when EnC finishes that it disposes the OOP connection it …
CyrusNajmabadi May 14, 2024
fe28ea8
Ensure that when EnC finishes that it disposes the OOP connection it …
CyrusNajmabadi May 14, 2024
73083b0
Simplify
CyrusNajmabadi May 14, 2024
a1d7845
Update src/EditorFeatures/Core/EditAndContinue/EditAndContinueLanguag…
CyrusNajmabadi May 14, 2024
066aa57
Merge branch 'main' into properDispose2
CyrusNajmabadi Jul 1, 2024
f61a54b
Merge branch 'main' into properDispose2
CyrusNajmabadi Jul 11, 2024
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
1 change: 1 addition & 0 deletions src/EditorFeatures/Core/EditAndContinue/EditAndContinueLanguageService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -262,6 +262,7 @@ public async ValueTask EndSessionAsync(CancellationToken cancellationToken)
UpdateApplyChangesDiagnostics([]);

sourceTextProvider.Deactivate();
// We don't need to Dispose _debuggingSession. That is done in EndDebuggingSessionAsync
_debuggingSession = null;
_committedDesignTimeSolution = null;
_pendingUpdatedDesignTimeSolution = null;
Expand Down
10 changes: 8 additions & 2 deletions src/Features/Core/Portable/EditAndContinue/Remote/RemoteDebuggingSessionProxy.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,19 @@
using Microsoft.CodeAnalysis.ErrorReporting;
using Microsoft.CodeAnalysis.Host;
using Microsoft.CodeAnalysis.Remote;
using Roslyn.Utilities;

namespace Microsoft.CodeAnalysis.EditAndContinue;

internal sealed class RemoteDebuggingSessionProxy(SolutionServices services, IDisposable? connection, DebuggingSessionId sessionId) : IActiveStatementSpanFactory, IDisposable
internal sealed class RemoteDebuggingSessionProxy(
SolutionServices services,
IReferenceCountedDisposable<IDisposable> connection,
Copy link
Member

@tmat tmat May 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Why passing in IReferenceCountedDisposable instead of the actual connection value like before? Once the constructor is reached it owns the object and is responsible for disposal. Indeed, you are throwing an unreachable exception if the underlying connection is not available.

Copy link
Member Author

Choose a reason for hiding this comment

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

ReferenceCountedDisposal can't be variant. And it wraps a specific type (not an IDisposable). The IRefCountedDisposable exists for this pattern where we want to pass a base interface type.

Copy link
Member

@sharwell sharwell Jul 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Once the constructor is reached it owns the object

This is sometimes the case, but sometimes not. It also doesn't work at all for asynchronous code that uses resources. RCD provides stronger/clearer semantics in the face of ownership transfers and asynchronous/concurrent operations.

Indeed, you are throwing an unreachable exception if the underlying connection is not available.

Callers are not expected to pass in an RCD which is not owned/alive by the caller. Unreachable seems like an appropriate exception here.

DebuggingSessionId sessionId) : IActiveStatementSpanFactory, IDisposable
{
private readonly IReferenceCountedDisposable<IDisposable> _connection = connection.TryAddReference() ?? throw ExceptionUtilities.Unreachable();

public void Dispose()
=> connection?.Dispose();
=> _connection.Dispose();

private IEditAndContinueService GetLocalService()
=> services.GetRequiredService<IEditAndContinueWorkspaceService>().Service;
Expand Down
24 changes: 14 additions & 10 deletions src/Features/Core/Portable/EditAndContinue/Remote/RemoteEditAndContinueServiceProxy.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
using Microsoft.CodeAnalysis.Host.Mef;
using Microsoft.CodeAnalysis.Remote;
using Microsoft.CodeAnalysis.Text;
using Roslyn.Utilities;

namespace Microsoft.CodeAnalysis.EditAndContinue;

Expand Down Expand Up @@ -130,25 +131,28 @@ private IEditAndContinueService GetLocalService()
if (client == null)
{
var sessionId = await GetLocalService().StartDebuggingSessionAsync(solution, debuggerService, sourceTextProvider, captureMatchingDocuments, captureAllMatchingDocuments, reportDiagnostics, cancellationToken).ConfigureAwait(false);
return new RemoteDebuggingSessionProxy(solution.Services, LocalConnection.Instance, sessionId);
using var disposable = new ReferenceCountedDisposable<IDisposable>(LocalConnection.Instance);
Copy link
Member Author

Choose a reason for hiding this comment

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

initial ref count will be 1.

Copy link
Member

Choose a reason for hiding this comment

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

Need to review this one

Copy link
Member

Choose a reason for hiding this comment

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

This one is potentially problematic, and doesn't follow the general rules for RCD. Resources can't be "partially" owned by RCD, so this should be rewritten to make it trivially clear to the reader that the value passed to the RCD instance is a new instance that was created as part of creating the RCD.

// RemoteDebuggingSessionProxy will add its own refcount to 'disposable'.
return new RemoteDebuggingSessionProxy(solution.Services, disposable, sessionId);
Copy link
Member Author

Choose a reason for hiding this comment

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

will increase ref count to '2', before the using lowers it back down to 1.

}

// need to keep the providers alive until the session ends:
var connection = client.CreateConnection<IRemoteEditAndContinueService>(
callbackTarget: new DebuggingSessionCallback(debuggerService, sourceTextProvider));
//
// Wrap with a ref-counted-disposable here to ensure we clean this up properly if we don't transfer ownership to the RemoteDebuggingSessionProxy.
using var connection = new ReferenceCountedDisposable<RemoteServiceConnection<IRemoteEditAndContinueService>>(
Copy link
Member Author

Choose a reason for hiding this comment

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

ref count of connection will start at 1.

Copy link
Member Author

Choose a reason for hiding this comment

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

this code ensures that if we don't properly do all the following steps that we dispose of hte connection. ew don't want connections leaked if we move to a world where we hold off on disposing the RemoteHostClient while there are still connections open to the OOP server (see #73467)

client.CreateConnection<IRemoteEditAndContinueService>(
callbackTarget: new DebuggingSessionCallback(debuggerService, sourceTextProvider)));

var sessionIdOpt = await connection.TryInvokeAsync(
var sessionIdOpt = await connection.Target.TryInvokeAsync(
solution,
async (service, solutionInfo, callbackId, cancellationToken) => await service.StartDebuggingSessionAsync(solutionInfo, callbackId, captureMatchingDocuments, captureAllMatchingDocuments, reportDiagnostics, cancellationToken).ConfigureAwait(false),
cancellationToken).ConfigureAwait(false);

if (sessionIdOpt.HasValue)
{
return new RemoteDebuggingSessionProxy(solution.Services, connection, sessionIdOpt.Value);
}
if (!sessionIdOpt.HasValue)
return null;

connection.Dispose();
return null;
// Pass the connection to the RemoteDebuggingSessionProxy which will add its own refcount to it.
return new RemoteDebuggingSessionProxy(solution.Services, connection, sessionIdOpt.Value);
Copy link
Member Author

Choose a reason for hiding this comment

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

it will increase to '2' inside RemoteDebuggingSessionProxy, then drop back to '1' when the using runs.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, this is the standard ownership transfer strategy for RCD

}

public async ValueTask<ImmutableArray<DiagnosticData>> GetDocumentDiagnosticsAsync(Document document, ActiveStatementSpanProvider activeStatementSpanProvider, CancellationToken cancellationToken)
Expand Down
Loading