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

[CyrusNajmabadi]

No description provided.

[CyrusNajmabadi]

@tmat ptal.

[CyrusNajmabadi]

initial ref count will be 1.

[CyrusNajmabadi]

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

[CyrusNajmabadi]

ref count of connection will start at 1.

[CyrusNajmabadi]

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)

[CyrusNajmabadi]

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

[CyrusNajmabadi]

@tmat are you ok with this? I still like this PR for the ensured cleanup durign the creation of these objects.

[tmat]

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?

[CyrusNajmabadi]

Adds a bit of extra burden on the use sites of OOP connection APIs.

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.

[CyrusNajmabadi]

@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.

[tmat]

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.

[tmat]

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.

[CyrusNajmabadi]

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.

[CyrusNajmabadi]

ReferenceCountedDisposable seems like an overkill.

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.

[tmat]

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;
        }
    }

[CyrusNajmabadi]

@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).

[tmat]

FWIW this is just working around using var being read-only.

using var x = ...;

Create(x);
x = null;

[CyrusNajmabadi]

@sharwell thoughts here? My preference is to not introduce a new type. But i'll go with teh majority here :)