Add DiagnosticsClientConnector to support creating a DiagnosticsClient from a diagnostic port

[xoofx]

Followup of #5070 that introduced a non-viable approach (Comment #5070 (comment))

This PR adds DiagnosticsClientConnector that is IAsyncDisposable and offers the method FromDiagnosticPort.

[xoofx]

I will try to add a test if this fix is accepted.

Question: Where should I add it in Microsoft.Diagnostics.NETCore.Client.UnitTests, new class or existing test?

[jander-msft]

This introduces a breaking change at compile time for those that have the CA2000 or CA2213 rules enabled. These are commonly enabled. This change would force everyone who have these rules enabled to dispose the client, regardless of if the disposal is really necessary.

This also will be problematic for tools that use the ReverseDiagnosticServer for more than one connection or more than one process at the same time e.g. dotnet-monitor.

IIRC, the original intent of the client is for it to be transiently used and should not manage the state of other components. It should be useable and then "tossable" without affecting the state of the server component.

Finally, I'm not a proponent of having unused files within classes like this because it creates ambiguity throughout the class as to whether it being null or not is valid for invocations of any of the methods. It will potentially lead to other bugs down the road as more changes are added that may depend on the field.

I would recommend revisiting the design of the convenience method that was added rather than changing the behavior of the client. I'm not on the .NET Diagnostics team, so I don't have the ultimate say on these changes. However, I do work on a partner team that relies heavily on this library for the tools that we build e.g. dotnet-monitor.

[xoofx]

I would recommend revisiting the design of the convenience method that was added rather than changing the behavior of the client. I'm not on the .NET Diagnostics team, so I don't have the ultimate say on these changes. However, I do work on a partner team that relies heavily on this library for the tools that we build e.g. dotnet-monitor.

So, with your knowledge, based on your usage, and if you didn't have access to the internals, what would you propose? 🙂

[xoofx]

I have changed the design from the original PR and added an entire new class DiagnosticsClientConnector separate from DiagnosticsClient that mitigate the issue with the disposal of the reversed server.

[mikem8361]

@jander-msft What do you think of his last changes?

[hoyosjs]

This makes more sense to me. It decouples reverse server lifetime from the client as @jander-msft had requested.

[hoyosjs]

@jander-msft do you have any more feedback? Otherwise I'll get this in this week