-
Notifications
You must be signed in to change notification settings - Fork 4k
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.
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
Ensure that when EnC finishes that it disposes the OOP connection it is holding onto #73465
base: main
Are you sure you want to change the base?
Conversation
@tmat ptal. |
@@ -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); |
There was a problem hiding this comment.
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.
return new RemoteDebuggingSessionProxy(solution.Services, LocalConnection.Instance, sessionId); | ||
using var disposable = new ReferenceCountedDisposable<IDisposable>(LocalConnection.Instance); | ||
// RemoteDebuggingSessionProxy will add its own refcount to 'disposable'. | ||
return new RemoteDebuggingSessionProxy(solution.Services, disposable, sessionId); |
There was a problem hiding this comment.
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.
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>>( |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
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); |
There was a problem hiding this comment.
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.
src/EditorFeatures/Core/EditAndContinue/EditAndContinueLanguageService.cs
Outdated
Show resolved
Hide resolved
src/EditorFeatures/Core/EditAndContinue/EditAndContinueLanguageService.cs
Outdated
Show resolved
Hide resolved
@tmat are you ok with this? I still like this PR for the ensured cleanup durign the creation of these objects. |
Adds a bit of extra burden on the use sites of OOP connection APIs. Can we instead build this logic into the RemoteServiceConnection type so that all services benefit without thinking about it? |
So users of OOP connection APis always had to dispose them when done. This PR at elast makes it so that we do that on the failure paths properly here, instead of potentially just leaking them out forever. |
@tmat seeking clarity on your original post :) i'd still like to take this. just for the cleanup hygiene on this complex initialization path. tnx. |
I see. I think I misunderstood to goal. Is there a simpler way of doing this though? ReferenceCountedDisposable seems like an overkill. It got me thinking we need to count references for some reason. But it is only used to dispose when we don't transfer ownership because the operation is canceled. |
internal sealed class RemoteDebuggingSessionProxy(SolutionServices services, IDisposable? connection, DebuggingSessionId sessionId) : IActiveStatementSpanFactory, IDisposable | ||
internal sealed class RemoteDebuggingSessionProxy( | ||
SolutionServices services, | ||
IReferenceCountedDisposable<IDisposable> connection, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
I'm not sure if we have a better pattern here. @sharwell ? If we don't have something tracking if ownership transfers, then we really need to have a ton of try/catches around everything to make sure that even in exceptional paths we dispose. I agree that this is a large concept (N ref counts) for what is effectively "are we at 0, 1, or t2 for the ref count". But it does work well for this. |
How about: // need to keep the providers alive until the session ends:
using var connection = DisposeScope.Create(client.CreateConnection<IRemoteEditAndContinueService>(
callbackTarget: new DebuggingSessionCallback(debuggerService, sourceTextProvider)));
var sessionIdOpt = await connection.Value.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.TransferOwnership(), sessionIdOpt.Value);
}
return null; static class DisposeScope
{
public static DisposeScope<T> Create<T>(T disposable) where T : IDisposable
=> new(disposable);
}
[NonCopyable]
struct DisposeScope<T>(T disposable) : IDisposable
where T : IDisposable
{
private bool _dispose = true;
public T Value => disposable;
public readonly void Dispose()
{
if (_dispose)
disposable.Dispose();
}
public T TransferOwnership()
{
_dispose = false;
return disposable;
}
} |
@sharwell thoughts on the above? i like it for being dedicated to this concept. I don't like it for sorta having two types very similar in goals (lifetimes). |
FWIW this is just working around
|
@sharwell thoughts here? My preference is to not introduce a new type. But i'll go with teh majority here :) |
No description provided.