Dispose transport on cancellation to prevent creation of new server stream/socket on shutdown of ReversedDiagnosticsServer.

[jander-msft]

@lateralusX observed an issue in the ReversedDiagnosticsServer where it will cause the underlying IpcServerTransport to recreate its stream/socket when the server is shutting down. The problem is caused by the token that is passed into ReversedDiagnosticsServer.ListenAsync is passed directly to IpcServerTransport.AcceptAsync; in both of the current implementations of IpcServerTransport.AcceptAsync, it will recreate the underlying stream/socket when the method call is cancelled. Since the IpcServerTransport is not disposed until after the call to AcceptAsync is cancelled, the IpcServerTransport implementation will create a new stream/socket and receive clients even though its about to be disposed. This leads to clients unnecessarily connecting to the server when it is shutting down.

The proposed fix is to dispose the IpcServerTransport implementation before the AcceptAsync call observes the cancellation. This allows the IpcServerTransport to call _cancellation.Cancel() internally, which causes AcceptAsync to cancel and NOT recreate the underlying stream/socket.

Alternative fix could be to add more state of IpcServerTransport to effectively say 'do not recreate stream/socket' which could then be exposed by a method (e.g. StopAcceptingClients). This would effectively cause the transport to become dormant. In my mind, this is the same as just disposing the IpcServerTransport since it would not be usable after setting this state, so I think just disposing it should be sufficient, especially given disposal will cancel all pending calls to AcceptAsync.

cc @lateralusX

[jander-msft]

I can look into adding a unit test that verifies that the server stream/socket is not recreated when ReversedDiagnosticsServer is shutting down.

[jander-msft]

I can look into adding a unit test that verifies that the server stream/socket is not recreated when ReversedDiagnosticsServer is shutting down.

I've added a unit test that counts the number of server transport creations before and after disposing of the ReversedDiagnosticsServer. If that count differs, then the test fails, which reflects that the fix was not sufficient.

[jander-msft]

If it's not desirable to call IpcServerTransport.Dispose explicitly, I can add a Stop method to IpcServerTransport that does most of what Dispose already does and then have ReversedDiagnosticsServer.ListenAsync and IpcServerTransport.Dispose call the Stop method.

[josalem]

I'm not entirely clear what the issue is here. The existing cancellation path should cancel the AcceptAsync call and then the dispose should shut down the transport at the end of ListenAsync. Is the concern that the OS transport remains open for new connections even though it won't call AcceptAsync again?

[jander-msft]

I'm not entirely clear what the issue is here. The existing cancellation path should cancel the AcceptAsync call and then the dispose should shut down the transport at the end of ListenAsync. Is the concern that the OS transport remains open for new connections even though it won't call AcceptAsync again?

Somewhat that. What happens is that IpcServerTransport.AcceptAsync is cancelled, which causes the named pipe or socket to be recreated. Any pending clients will be disconnected from the previous named pipe or socket, probe for if the named pipe or socket is available again, and reconnect to the new one. The ListenAsync call then disposes IpcServerTransport, which eventually disposes the named pipe or socket, and forces the clients to be disconnected again.

This reconnect and subsequent disconnect by clients is unnecessary because the server is going down. Thus, we can shutdown the IpcServerTransport instead of just relying on the cancellation in order to prevent this jittery reconnection by clients.

[sywhang]

LGTM. Thanks!