Can this code have bottleneck or be resource-intensive?

Question

It's code that will execute 4 threads in 15-min intervals. The last time that I ran it, the first 15-minutes were copied fast (20 files in 6 minutes), but the 2nd 15-minutes are much slower. It's something sporadic and I want to make certain that, if there's any bottleneck, it's in a bandwidth limitation with the remote server.

EDIT: I'm monitoring the last run and the 15:00 and :45 copied in under 8 minutes each. The :15 hasn't finished and neither has :30, and both began at least 10 minutes before :45.

Here's my code:

static void Main(string[] args)
{

    Timer t0 = new Timer((s) =>
    {
        Class myClass0 = new Class();
        myClass0.DownloadFilesByPeriod(taskRunDateTime, 0, cts0.Token);
        Copy0Done.Set();
    }, null, TimeSpan.FromMinutes(20), TimeSpan.FromMilliseconds(-1));

    Timer t1 = new Timer((s) =>
    {
        Class myClass1 = new Class();
        myClass1.DownloadFilesByPeriod(taskRunDateTime, 1, cts1.Token);
        Copy1Done.Set();
    }, null, TimeSpan.FromMinutes(35), TimeSpan.FromMilliseconds(-1));

    Timer t2 = new Timer((s) =>
    {
        Class myClass2 = new Class();
        myClass2.DownloadFilesByPeriod(taskRunDateTime, 2, cts2.Token);
        Copy2Done.Set();
    }, null, TimeSpan.FromMinutes(50), TimeSpan.FromMilliseconds(-1));

    Timer t3 = new Timer((s) =>
    {
        Class myClass3 = new Class();
        myClass3.DownloadFilesByPeriod(taskRunDateTime, 3, cts3.Token);
        Copy3Done.Set();
    }, null, TimeSpan.FromMinutes(65), TimeSpan.FromMilliseconds(-1));

}

public struct FilesStruct
{
    public string RemoteFilePath;
    public string LocalFilePath;
}

Private void DownloadFilesByPeriod(DateTime TaskRunDateTime, int Period, Object obj)
{

    FilesStruct[] Array = GetAllFiles(TaskRunDateTime, Period);
    //Array has 20 files for the specific period.

    using (Session session = new Session())
    {
        // Connect
        session.Open(sessionOptions);
        TransferOperationResult transferResult;
        foreach (FilesStruct u in Array)
        {
            if (session.FileExists(u.RemoteFilePath)) //File exists remotely
            {
                if (!File.Exists(u.LocalFilePath)) //File does not exist locally
                {
                    transferResult = session.GetFiles(u.RemoteFilePath, u.LocalFilePath);
                    transferResult.Check();
                    foreach (TransferEventArgs transfer in transferResult.Transfers)
                    {
                        //Log that File has been transferred
                    }
                }
                else
                {
                    using (StreamWriter w = File.AppendText(Logger._LogName))
                    {
                        //Log that File exists locally
                    }
                }

            }
            else
            {
                using (StreamWriter w = File.AppendText(Logger._LogName))
                {
                    //Log that File exists remotely
                }
            }
            if (token.IsCancellationRequested)
            {
                break;
            }
        }
    }
}

Answer 1

Something is not quite right here. First thing is, you're setting 4 timers to run parallel. If you think about it, there is no need. You don't need 4 threads running parallel all the time. You just need to initiate tasks at specific intervals. So how many timers do you need? ONE.

The second problem is why TimeSpan.FromMilliseconds(-1)? What is the purpose of that? I can't figure out why you put that in there, but I wouldn't.

The third problem, not related to multi-programming, but I should point out anyway, is that you create a new instance of Class each time, which is unnecessary. It would be necessary if, in your class, you need to set constructors and your logic access different methods or fields of the class in some order. In your case, all you want to do is to call the method. So you don't need a new instance of the class every time. You just need to make the method you're calling static.

Here is what I would do:

1. Store the files you need to download in an array / List<>. Can't you spot out that you're doing the same thing every time? Why write 4 different versions of code for that? This is unnecessary. Store items in an array, then just change the index in the call!
2. Setup the timer at perhaps 5 seconds interval. When it reaches the 20 min/ 35 min/ etc. mark, spawn a new thread to do the task. That way a new task can start even if the previous one is not finished.
3. Wait for all threads to complete (terminate). When they do, check if they throw exceptions, and handle them / log them if necessary.
4. After everything is done, terminate the program.

For step 2, you have the option to use the new async keyword if you're using .NET 4.5. But it won't make a noticeable difference if you use threads manually.

And why is it so slow...why don't you check your system status using task manager? Is the CPU high and running or is the network throughput occupied by something else or what? You can easily tell the answer yourself from there.

Answer 2

The problem was the sftp client.

The purpose of the console application was to loop through a list<> and download the files. I tried with winscp and, even though, it did the job, it was very slow. I also tested sharpSSH and it was even slower than winscp.

I finally ended up using ssh.net which, at least in my particular case, was much faster than both winscp and sharpssh. I think the problem with winscp is that there was no evident way of disconnecting after I was done. With ssh.net I could connect/disconnect after every file download was made, something I couldn't do with winscp.