DicomServer does not close TCP connection

[mia-san]

reported by @cjcaldwell via gitter

Expected behavior

DicomServer closes TCP connection after client closed connection.

Actual behavior

DicomServer leaves TCP connection open after client closed connection.

Steps to reproduce the behavior

send C-ECHO request repeatedly to DicomServer and observe sockets.
see additional comments.

fo-dicom version and OS/platform

4.0.0

• while DicomServer.Dispose() does not close TCP listen connection. #158 seems similar, network codes are getting evolved since then, this issue seems to be different one.

• @cjcaldwell spotted that TcpClient does not get closed.

  DesktopNetworkListener receives TcpClient into local variable.
  

  fo-dicom/DICOM/Network/DesktopNetworkListener.cs

  Line 71 in c1abfb8

  var tcpClientTask = awaiter as Task<TcpClient>;

  TcpClient is passed to DesktopNetworkStream's constructor.
  but DesktopNetworkStream does not store the reference.
  

  fo-dicom/DICOM/Network/DesktopNetworkListener.cs

  Line 82 in c1abfb8

  return new DesktopNetworkStream(tcpClient, _certificate);

  now, no one has reference to the TcpClient.
  TcpClient leaks without closing.

• WindowsNetworkListener seems to be leaking socket in slightly different manner also.

  WindowsNetworkListener passes new socket to WindowsNetworkStream's constructor.
  

  fo-dicom/DICOM/Network/WindowsNetworkListener.cs

  Line 115 in c1abfb8

  _streamTcs?.TrySetResult(new WindowsNetworkStream(_socket));

  WindowsNetworkStream's constructor stores the socket into member variable.
  

  fo-dicom/DICOM/Network/WindowsNetworkStream.cs

  Line 84 in c1abfb8

  this.socket = socket;

  however, canDisposeSocket is set to false.
  

  fo-dicom/DICOM/Network/WindowsNetworkStream.cs

  Line 85 in c1abfb8

  this.canDisposeSocket = false;

  which makes socket leave open.
  

  fo-dicom/DICOM/Network/WindowsNetworkStream.cs

  Line 144 in c1abfb8

  if (this.canDisposeSocket && this.socket != null)

  now socket leaks without closing.

• comment on DesktopNetworkListener's constructor says that closing TcpClient is caller's responsibility.
  

  fo-dicom/DICOM/Network/DesktopNetworkStream.cs

  Line 80 in c1abfb8

  /// <remarks>Ownership of <paramref name="tcpClient"/> remains with the caller, including responsibility for

  probably it was at the moment the comment was written, but it is suspicious now.

  WindowsNetworkStream's constructor also has similar comment.
  

  fo-dicom/DICOM/Network/WindowsNetworkStream.cs

  Line 74 in c1abfb8

  /// <remarks>Ownership of <paramref name="socket"/> remains with the caller, including responsibility for

thank you again to @cjcaldwell

[mia-san]

//
//  sample C-ECHO SCP
//
using Dicom.Log;
using Dicom.Network;
using System;

namespace ConsoleApp1
{
    internal class Program
    {
        public static void Main(string[] args)
        {
            LogManager.SetImplementation(ConsoleLogManager.Instance);

            using (var server = new DicomServer<DicomCEchoProvider>())
            {
                server.StartAsync(null, 12345, null, null, null, null)
                    .GetAwaiter()
                    .GetResult();
            }

            Console.ReadLine();
        }
    }
}

[mia-san]

REM
REM request C-ECHO repeatedly with dcmtk echoscu.exe
REM

SET ECHOSCU_PROGRAM=echoscu.exe
SET ECHOSCU_HOST=127.0.0.1
SET ECHOSCU_PORT=12345
SET ECHOSCU_CALLING=ANY-SCU
SET ECHOSCU_CALLED=ANY-SCP

FOR /L %%c IN (1, 1, 100) DO %ECHOSCU_PROGRAM% --aetitle %ECHOSCU_CALLING% --call %ECHOSCU_CALLED% %ECHOSCU_HOST% %ECHOSCU_PORT%

netstat -na | find "%ECHOSCU_PORT%"

pause

[mia-san]

I found that just letting NetworkStream to keep reference to socket or TcpClient is not enough.

DicomServer takes DicomService-derived class as type parameter.

fo-dicom/DICOM/Network/DicomServer.cs

Line 21 in c1abfb8

public class DicomServer<T> : IDicomServer<T> where T : DicomService, IDicomServiceProvider

DicomService-derived class is instantiated by CreateScp.

fo-dicom/DICOM/Network/DicomServer.cs

Line 219 in c1abfb8

var instance = (T)Activator.CreateInstance(typeof(T), stream, _fallbackEncoding, Logger);

scp is assigned to local variable

fo-dicom/DICOM/Network/DicomServer.cs

Line 248 in c1abfb8

var scp = CreateScp(networkStream);

scp's RunAsync is invoked, and scp variable is just forgotten.

fo-dicom/DICOM/Network/DicomServer.cs

Line 254 in c1abfb8

_services.Add(scp.RunAsync());

this means that DicomService-derived class would only be disposed via finalizer.

we have to make DicomServer to dispose instance of class T as well.