Handle leak when connection to SSH server fails

[zhanghfeng]

I find handle leak occurred when use sshClient to connect a linux machine which power status is OFF.

occurred conditions & version

target linux machine is power off and version is 2016.0.0

handle information

I use the process explorer tool to analyze the issue. I find that \Device\Afd is continuous increase.

• Type:File
• Name:\Device\Afd

sample

using Renci.SshNet;
using Renci.SshNet.Common;
using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading;
using System.Threading.Tasks;

namespace SSHNETTEST
{
    class Program
    {
        static void Main(string[] args)
        {
            string host = "192.168.154.201";//IP Address of connect target linux machine, and power off this machine
            string username = "root";//user name 
            string password = "abc123$%^";//password
            for (var i = 0; i < 1000000; i++)
            {
                SshClient sshClient = new SshClient(host, 22, username, password);
                try
                {
                    sshClient.Connect();
                    sshClient.Dispose();
                }
                catch
                {
                    sshClient.Dispose();
                }
                Thread.Sleep(3 * 1000);
            }
        }
    }
}

[drieseng]

I've looked into it, and I think I can easily fix the file handle leak.
But there still appear to be leaks.
Do you have time to work with me on this ?
I propose to create a minimal app that allows us to reproduce this issue, and start working from there.

[zhanghfeng]

OK, I will assist you to do some test work.

[drieseng]

I've located the leaks.
I'll do some more tests, and commit the fixes later this week.
Would you be willing to test an early previous of the next release ?
Either I send you the assembly through email, or you can build from source.

[zhanghfeng]

OK. I will test the case and report my test result. I will build from source after you commit the code.

[drieseng]

Please confirm that 9387469 fixes this problem.

[zhanghfeng]

Thanks.

I had test the modified code. The ISSUE that \Device\Afd is continuous increase had been fixed. but there also have handle leaks. I use process explorer to analyze the leaks. I find that Thread Handle is continuous increase.

• Type: Thread
• Name: Process Name(ProcessID)：ThreadID
  • example: SSHNETTEST.exe(21228):20952

And I use GC.Collect to do garbage collection every 10 minutes, I found no Handle leaks.

[woric]

Hi guys,

Sorry to comment without writing any code, but it's late and I am writing this on my phone.

Firstly, the comments above are an indication of Disposable objects not being correctly disposed. Thread handles, like all Disposable objects, are resources that must be properly managed or they will cause resource leaks.

The rules for avoiding the resources leaks are fairly simple, and when followed correctly, they will always solve the problem. If the problem persists, then there is a Disposable object not being correctly disposed somewhere in the code.

What is required is that for every instance of any class that has a Dispose method you must do the following...

1. either call Dispose directly, or use a using statement. (A using statement is simply a try/finally wrapper that calls Dispose for you. They also make the code much more readable)

2. If the instance is contained within a single method then use a using statement. On the rare cars that you cannot use a using statement, then call Dispose in the finally block.

3. If the instance is a member a class then that class MUST implement the IDisposable interface and correctly call Dispose in the Dispose method of the class. This is non-negotiable and there are many online pages on how to best implement IDisposable. (I also recommend making the Disposable member private to avoid it accidentally being disposed of incorrectly used)

4. Never rely on the Garbage Collector to do any of this for you. If you do then you may run out of thread handles.

5. Calling the Garbage Collector via code is not a good solution.

If you religiously follow those rules on every instance of any class that has a Dispose method then your resource leaks will disappear.

[drieseng]

@zhanghfeng Indeed, I noticed that myself. If you run in debug configuration, then both the number of handles and the number of threads increase (if you don't force a GC).

When you run it release configuration, the number of handles remains stable, but the number of threads still increases.

[drieseng]

@woric Thanks for the .NET Development 101 :-)

Here's a minimal repro:

using System;
using System.Globalization;
using System.Net;
using System.Net.Sockets;
using System.Threading;

class Program
{
    static void Main()
    {
        var endPoint = new IPEndPoint(IPAddress.Parse("192.168.154.201"), 22);
        var timeout = TimeSpan.FromMilliseconds(1000);

        for (var i = 0; i < 5000; i++)
        {
            try
            {
                using (var socket = Connect(endPoint, timeout))
                    socket.Dispose();
            }
            catch (Exception ex)
            {
                Console.WriteLine(i + " => " + ex);
            }
        }

        Console.ReadKey();
    }

    private static Socket Connect(EndPoint endPoint, TimeSpan timeout)
    {
        var socket = new Socket(endPoint.AddressFamily, SocketType.Stream, ProtocolType.Tcp) { NoDelay = true };

        var connectCompleted = new ManualResetEvent(false);
        var args = new SocketAsyncEventArgs
        {
            UserToken = connectCompleted,
            RemoteEndPoint = endPoint
        };
        args.Completed += ConnectCompleted;

        if (socket.ConnectAsync(args))
        {
            if (!connectCompleted.WaitOne(timeout))
            {
                // avoid ObjectDisposedException in ConnectCompleted
                args.Completed -= ConnectCompleted;
                // avoid leaking /Device/Afd handles
                Socket.CancelConnectAsync(args);
                // dispose Socket
                socket.Dispose();
                // dispose ManualResetEvent
                connectCompleted.Dispose();
                // dispose SocketAsyncEventArgs
                args.Dispose();

                throw new TimeoutException(string.Format(CultureInfo.InvariantCulture,
                    "Connection failed to establish within {0:F0} milliseconds.", timeout.TotalMilliseconds));
            }
        }

        // dispose ManualResetEvent
        connectCompleted.Dispose();

        if (args.SocketError != SocketError.Success)
        {
            var socketError = (int) args.SocketError;

            // dispose Socket
            socket.Dispose();
            // dispose SocketAsyncEventArgs
            args.Dispose();

            throw new SocketException(socketError);
        }

        // dispose SocketAsyncEventArgs
        args.Dispose();

        return socket;
    }

    private static void ConnectCompleted(object sender, SocketAsyncEventArgs e)
    {
        var eventWaitHandle = (ManualResetEvent) e.UserToken;
        if (eventWaitHandle != null)
            eventWaitHandle.Set();
    }
}

Now show me the money, and fix the leak :p

[drieseng]

@stephentoub Would you be willing to take a look at the repro code in my previous comment, and tell me why it leaks threads (unless you force GC) ? Thx!!