FTP File Upload Queue

Question

This code implements an FTPManager class and is designed to be a place to insert file data to be sent later, without having the program wait for a potentially very slow file operation to finish.

• This code is designed to work around some issues that I've been noticing when having multiple threads use FTPWebRequest simultaneously (note that each thread had its own instance of FTPWebRequest).

• It's also designed with the intention of reducing the amount of connections my application needs to a FTP Server. With this implementation my application only has a single FTP connection, though it doesn't re-use it -- should it?

• Note that this application is targeting .NET 4.0. I can't upgrade to .NET 4.5+ at this time.

• File throughput is not a huge issue, as long as the files get there "eventually"; transfers that fail must be logged.

• Files may be added at a rate of over 100 files per second, most of them will be a few MB, but they will not exceed around 20 MB. AddJob should be executed quickly so the thread saving the file can move on to other tasks.
• The user has the option to have the program wait for the file transfer to complete, in that case I would use SendDataToFTP_Throws instead of AddJob. Note that the calling function is prepared to handle this function's exceptions, in a way that's more than just logging it.
• The user does not need any progress indicators, this is happening completely in the background.
• Testing so far: This code has been running for a few days 24/7, at around 10 files per second, with five threads calling AddJob. I haven't run into any problems yet. It has ran over a million files.
• Comments that were added for the sake of this code review have @CR in them, they bring up specific concerns that I'd like feedback on.
• The test code assumes that you have a FTP Server running on your local computer, on port 21 that allows Anonymous access, and that there is a folder called testfolder on the server.
• For the test, all errors are logged to the console, but there is a more sophisticated setup in place for the application that this hooks into.

using System;
using System.Collections.Generic;
using System.Collections.Concurrent;
using System.Threading.Tasks;
using System.Net;

using System.Windows.Forms; // for test code
using System.IO;            // for test code
using System.Diagnostics;   // for test code

namespace FTPTest2
{
    class Program
    {
        const int cNumberOfFilesToSend = 10;
        const string cFTPfolderPath = "ftp://localhost:21/testfolder/";
        const string cFTPUsername = "Anonymous";
        const string cFTPPassword = "";

        private static byte[] FileBytes;

        // testing functions (for example usage). Sends copies of the selected file to a ftp server.
        [STAThread]
        static void Main(string[] args)
        {

            FTPManager.StartTransferBackgroundTask();

            OpenFileDialog choosefiletosenddialog = new OpenFileDialog();
            DialogResult dialogresult = choosefiletosenddialog.ShowDialog();

            if (dialogresult != DialogResult.OK)
            {
                return;
            }

            string selectedfilepath = choosefiletosenddialog.FileName;


            string selectedfilename = Path.GetFileNameWithoutExtension(selectedfilepath);
            string selectedfileextension_withdot = Path.GetExtension(selectedfilepath);

            using(FileStream selectedfilestream = new FileStream(selectedfilepath, FileMode.Open))
            {
                // note: a 3rd party API only allows me to get data using a byte array
                if (selectedfilestream.Length > int.MaxValue)
                {
                    MessageBox.Show("Selected file is too big.");
                    return;
                }

                FileBytes = new byte[selectedfilestream.Length];

                selectedfilestream.Read(FileBytes, 0, (int) selectedfilestream.Length);
            }

            // note that FTPWebrequest would fail this test if I sent 100,000 files using a threadpool for each file
            // transfer operation.
            for (int fileindex = 0; fileindex < cNumberOfFilesToSend; fileindex++)
            {
                string filename = String.Format("{0}_{1}{2}", selectedfilename, fileindex.ToString(), selectedfileextension_withdot);
                System.Threading.ThreadPool.QueueUserWorkItem(Thread_Main, filename);

            }

            Console.WriteLine("Waiting for all jobs to finish...");

            while (!FTPManager.AllJobsComplete())
            {
                System.Threading.Thread.Sleep(100);
            }

            Console.WriteLine("All jobs complete. Hit Enter to quit...");
            Console.ReadLine();
        }

        static void Thread_Main(object filename)
        {
            if (!(filename is string))
            {

                Debug.Fail("Invalid parameter type");
                return;
            }

            FTPManager.AddJob((string) filename, FileBytes, cFTPfolderPath, cFTPUsername, cFTPPassword);
        }

    }


    class Stub
    {
        public static void Log_Error(string message)
        {
            Console.WriteLine(message);
        }
    }

    internal class FTPManager
    {
        // note: for the sake of this test this was set lower than normal.
        // 10000 is more reasonable on some networks.
        internal const int cFTPTimeout_ms = 1000;

        private static ConcurrentQueue<FTPFileJob> FTPJobs = new ConcurrentQueue<FTPFileJob>();
        private static Task FTPTransferTask;

        // @CR: What do you guys think of this? The .NET framework reference source occasionally makes internal exceptions.
        internal class GenericFTPException : Exception
        {
            public GenericFTPException() : base() { }

            public GenericFTPException(string message) : base(message) { }

            public GenericFTPException(string message, Exception inner) : base(message, inner) { }
        }

        private class FTPFileJob
        {
            internal readonly string FileName;
            internal readonly byte[] Data;
            internal readonly string FTPFolderUrl;
            internal readonly string Username;
            internal readonly string Password;

            internal FTPFileJob(string filename, byte[] filedata, string ftpfolderurl, string username, string password)
            {
                this.FileName = filename;
                this.Data = filedata;
                this.FTPFolderUrl = ftpfolderurl;
                this.Username = username;
                this.Password = password;

             }        
        }

        internal static void StartTransferBackgroundTask()
        {
            if (FTPManager.FTPTransferTask != null)
            {
                return;
            }

            FTPManager.FTPTransferTask = new Task(ProcessJobs);
            FTPManager.FTPTransferTask.Start();
        }

        private static void ProcessJobs()
        {
            try
            {
                while (true)
                {

                    FTPFileJob ftpjob;

                    if (FTPManager.FTPJobs.TryDequeue(out ftpjob))
                    {
                        FTPManager.SendJob(ftpjob);
                    }

                    else
                    {
                        System.Threading.Thread.Sleep(100);
                    }

                 }
            }
            catch (System.Threading.ThreadInterruptedException)
            {
            }
        }

        internal static void AddJob(string filename, byte[] filedata, string ftpfolderurl, string username, string password)
        {
            FTPManager.FTPJobs.Enqueue(new FTPFileJob(filename, filedata, ftpfolderurl, username, password));
        }

        internal static bool AllJobsComplete()
        {
            return FTPManager.FTPJobs.IsEmpty;
        }

        private static void SendJob(FTPFileJob ftpjob )
        {
            try
            {
                FTPManager.SendDataToFTP_Throws(ftpjob.Data, ftpjob.FTPFolderUrl, ftpjob.FileName, ftpjob.Username, ftpjob.Password);
            }

            catch(GenericFTPException ftpexception)
            {
                Stub.Log_Error(ftpexception.Message);
            }
        }

        internal static void SendDataToFTP_Throws(byte [] data, string ftpfolderurl, string filename, string username, string password)
        {
            FtpWebRequest ftpwebrequest;
            string filepath = ftpfolderurl + filename;

            try
            {
                ftpwebrequest = (FtpWebRequest) FtpWebRequest.Create(ftpfolderurl + filename);
                ftpwebrequest.Method = WebRequestMethods.Ftp.UploadFile;
                ftpwebrequest.Credentials = new NetworkCredential(username, password);

                ftpwebrequest.Timeout = cFTPTimeout_ms;
                ftpwebrequest.UseBinary = true;
                ftpwebrequest.KeepAlive = false;

                //read in data from the file to a byte buffer
                using(System.IO.Stream tWebRequestWriterStream = ftpwebrequest.GetRequestStream())
                {
                    tWebRequestWriterStream.Write(data, 0, data.Length);

                }

                using(FtpWebResponse response = (FtpWebResponse) ftpwebrequest.GetResponse())
                {
                    //note that ClosingData is the status code returned if everything's ok
                    if (response.StatusCode == FtpStatusCode.ClosingData)
                    {
                        //success!
                    }

                    else
                    {
                        throw new GenericFTPException("File '" + filepath + "' upload to FTP failed: (" + response.StatusCode + "): " + response.StatusDescription);
                    }
                }
            }
            catch(System.IO.IOException ioexception) 
            {
                //this will occur if:
                //- I kick the user that's transferring the file using the Filezilla server admin window
                throw new GenericFTPException("Failed to upload file '" + filepath + "' to FTP: " + ioexception.Message, ioexception);
            }
            catch(System.Net.WebException webexception)
            {
                //this will occur if:
                //- You try to send a file that is already on the server
                //- The server can't be reached
                throw new GenericFTPException("Failed to upload file '" + filepath + "' to FTP: " + webexception.Message, webexception);
            }
            catch(ArgumentOutOfRangeException argumentoutofrangeexception)
            {
                //This is a strange one, it used to happen sporadically on the OLD (multithreaded) code, it doesn't seem to happen now.
                //I would get a long string of FTP timeouts, followed by this exception.
                //see this post for more details: https://social.msdn.microsoft.com/Forums/vstudio/en-US/5f4e0689-75bf-49f5-9854-d12e79aae84f/ftpwebrequest-argumentoutofrangeexception-millisecondstimeout?forum=netfxbcl
                throw new GenericFTPException("Failed to upload file '" + filepath + "' to FTP: .NET Framework internal FTP indexing error. Full info:\n\n" + argumentoutofrangeexception.ToString());
            }
        }
    }
}

// note: This code can be considered public domain, or the absolute minimum licensing restrictions that CR allows. If you like it feel free to use it.

Note that I am mostly hoping for feedback on coding style, though any possible suggestions for simplification, or threading mistakes or possible bugs I missed would be appreciated too.

I'd tag this "readability" but that tag doesn't seem to be available.

Answer 1

Nested classes

Nesting classes is usually a bad idea. I would try to avoid.

FTPFileJob

You have created this class to group some data about the upload... then you should be consistent are reuse it where appropriate instead of splitting it and passing each property one by one.

FTPManager.SendDataToFTP_Throws(ftpjob.Data, ftpjob.FTPFolderUrl, ftpjob.FileName, ftpjob.Username, ftpjob.Password);

Make the SendDataToFTP_Throws accept a FTPFileJob.

Throws suffix

We usually use such suffixes only in tests.

SendJob

This method is unnecessary. All it does is split the parameters and catch the exception. You can as well add the logging to the SendDataToFTP_Throws method, change its signature and get rid of SendJob.

Constants

In C# we don't use prefixes for constants. We name them just like any other property with ``PascalCase`.

try/catch

catch (System.IO.IOException ioexception)
{
  //this will occur if:
  //- I kick the user that's transferring the file using the Filezilla server admin window
  throw new GenericFTPException("Failed to upload file '" + filepath + "' to FTP: " + ioexception.Message, ioexception);
}
catch (System.Net.WebException webexception)
{
  //this will occur if:
  //- You try to send a file that is already on the server
  //- The server can't be reached
  throw new GenericFTPException("Failed to upload file '" + filepath + "' to FTP: " + webexception.Message, webexception);
}

I don't see much value in splitting the exception handling here as both catches do the same thing. The only difference are the comments. You should log them or put them in the message so know the reason later without looking at the code.

argumentoutofrangeexception

In C# we use camelCase for variable names and not alllowercase.

OpenFileDialog

This is a console app so it might not really matter but the OpenFileDialog is IDisposable.

xManager

Manager is not usually a good name. I'd call it FtpUploader.