1
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Are you uploading each file into a different folder and server because you specify those parameters for each call (job)? \$\endgroup\$ – t3chb0t Aug 25 '16 at 19:40
  • \$\begingroup\$ @t3chb0t each transfer could go to a different folder / server/ port using different credentials, this is a design requirement, each thread could be sending to a different server, or a single thread could even be sending several files to different servers. Also the server / credentials are user editable and can vary during runtime. \$\endgroup\$ – jrh Aug 25 '16 at 19:50
  • \$\begingroup\$ Update: After about a week's worth of running I notice that this code throws InternalException exceptions; I think this is due to running FTPManager's upload Task on a different thread, because I ran 43 million files over a week, on a version of this code that ran on STAThread without a single exception being thrown. I recommend reading this article, it mentions some problems in FTPWebrequest. I am going to do another week's worth of files on this stock code to narrow it down then post an answer with the results. \$\endgroup\$ – jrh Oct 21 '16 at 16:47
  • \$\begingroup\$ Related (old-ish) discussion on Chat: chat.stackexchange.com/transcript/message/32150727#32150727 \$\endgroup\$ – jrh Oct 21 '16 at 16:56
  • \$\begingroup\$ Note that storing the FTP folder URL as a string is not a smart idea in my opinion, FTPWebRequest converts it to a Uri internally anyway, and in retrospect I wish I would have just created the Uri ahead of time because it includes some rather nice validation \$\endgroup\$ – jrh Jan 20 '17 at 16:04
2
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Thanks for the review, one thing though, unfortunately WebException does not inherit from IOException, msdn.microsoft.com/en-us/library/… , I might be able to use "when" but that's not available in C# 4.0, so the only way to combine these into one statement involves catch (Exception ex). \$\endgroup\$ – jrh Aug 25 '16 at 21:24
  • \$\begingroup\$ "Manager is not usually a good name." Fun fact: Roslyn seems to use a lot of xManager classes. \$\endgroup\$ – jrh Jan 24 '18 at 21:24
  • 1
    \$\begingroup\$ @jrh no wonder, there are 43 contributors and such projects as asp-core or roslyn are an example of a lot of bad pratices, terrible ideas, unbelivable repetitions and amateur code ;-] I no longer use it as an inspiration. \$\endgroup\$ – t3chb0t Jan 25 '18 at 5:21
  • \$\begingroup\$ yep. There are cool things in there but the code has some room for improvement in places. It would be nice if Roslyn could be a great example of how to code in C#, but it's not quite that. Sometimes it's more of a "lessons learned" codebase... \$\endgroup\$ – jrh Jan 25 '18 at 14:03

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.