Multi threading with webrequests/responses

Question

I have been working with multi threading and would like some feedback on the code, i generally suck at class design/OO which i am trying to improve. Im sure there is a better way of making it neater and maximizing readability.

using System;
using System.Collections.Generic;
using System.IO;
using System.Linq;
using System.Net;
using System.Text;
using System.Threading;
using System.Threading.Tasks;
using System.Xml;

namespace XMLSender
{
    class Program
    {
        private static string serverUrl;
        static void Main(string[] args)
        {
            Console.WriteLine("Please enter the URL to send the XML File");
            serverUrl = Console.ReadLine();

            List<Thread> threads = new List<Thread>();
            List<string> names = new List<string>();
            string fileName = "";

            do
            {
                Console.WriteLine("Please enter the XML File you Wish to send, To start simulation type 'start'");
                fileName = Console.ReadLine();
                if (fileName != "start")
                {
                    Thread t = new Thread(new ParameterizedThreadStart(send));
                    threads.Add(t);
                    names.Add(fileName);
                }
            }
            while (fileName != "start");

            for (int i = 0; i < threads.Count; i++)
            {
                var t = threads[i];
                var name = names[i];
                t.Start(name);
            }

            foreach (Thread t in threads)
            {
                t.Join();
            }

        }
        static private void send(object data)
        {
            try
            {
                //Set the connection limit of HTTP
                System.Net.ServicePointManager.DefaultConnectionLimit = 10000;

                //ServicePointManager.ServerCertificateValidationCallback = delegate { return true; };               
                HttpWebRequest request = (HttpWebRequest)WebRequest.Create(http://www.contoso.com/example.aspx);

                byte[] bytes;  

                //Send XML data to Webserver
                bytes = File.ReadAllBytes((string)data);
                request.ContentType = "text/xml; encoding='utf-8'";
                request.ContentLength = bytes.Length;
                request.Method = "POST";
                //request.Timeout = 20000;
                request.KeepAlive = false;

                using (Stream requestStream = request.GetRequestStream())
                {
                    requestStream.Write(bytes, 0, bytes.Length);
                    requestStream.Close();
                }

                // Get response from Webserver
                using (HttpWebResponse response = (HttpWebResponse)request.GetResponse())
                {
                    Stream responseStream = response.GetResponseStream();
                    string responseStr = new StreamReader(responseStream).ReadToEnd();
                    Console.WriteLine(responseStr + Environment.NewLine);
                    responseStream.Close();
                }


            }
            catch (Exception e)
            {
                Console.WriteLine("An Error Occured" + Environment.NewLine + e);
                Console.ReadLine();
            }
        }
    }
}

Answer 1

You say you suck at class design but there's only one class in your code and everything is very "linear". I'm no expert anyway, but I'll try to help.

First of all you should separate the Main() function from the code you're executing (unlesss the code is short), this will improve readbility at it will be easier to update the code. To do this you can create a class where the main work will happen.

On the Main() function:

serverUrl = Console.ReadLine();

If you have many variables it might be better to declare this at the start of the function, but it isn't always necessary. In my opinion on the top you should put the variables that will be used many times or that are important.

while (fileName != "start");

I would do this with an if but this works fine. What I would also do is to save all the inputs in a list, either a List<> or a string[]. Also you're doing the "start" checking twice, one on the loop, and one on the code inside the loop.

It's simpler like this:

List<string> fileNames = new List<string>();
while (currentLine != "start")
    fileNames.Add(currentLine);

After this you iterate thorugh the items 2 times, I will go on that later.

Now we can create a class that does the "heavy work". Let's ask ourselves: what do we want this class to do? And what does it need to do that?

Well, now you have the class functions and its params. We want the class to send data on, it needs the "XML file path", and we want to do it on a thread.

 class TestClass
{
    private string xmlPath;
    public TestClass(string _xmlPath)
    {
        xmlPath = _xmlPath;
    }
    public void SendDataAsync(object data)
    {

    }
    public void SendData(object data)
    {

    }
}

We create a local class variable that's asked when we create the class and that will be accesed later: xmlPath.

Sometimes, using Threads it's nasty and can cause problems and performance issues (if using many of them, so I like using Tasks. This is what will be happening on the SendDataAsyncFunction:

 public void SendDataAsync(object data)
        => Task.Run(() => SendData(data));     

If we only have one line of code using => makes it simpler than using brackets.

Then on the SendData function:

On the first line we see a try block, then is it really necessary? What could throw an exception? Things that could throw and exception are File.ReadAllBytes , request.Write and the response, in this case it's ok to have a try block on the whole function, but normally you shouldn't do that.

 System.Net.ServicePointManager.DefaultConnectionLimit = 10000;

Is it necessary to do this every time? Why not take this to the main function? Since it's only executed once.

Then the bytes are read, to reduce code lines we can do this:

 byte[] bytes = File.ReadAllBytes(xmlPath);

Next the HttpResquest is created, if we take this to another function (ideally each function should do only 1 thing), the code will be easier to read and if something has to be change less problems will occur.

 private HttpWebRequest CreateRequest(int length)
    {
        WebRequest request = WebRequest.Create(ATMSimulator.Properties.Settings.Default.KTHBaseUri);
        request.ContentType = "text/xml; encoding='utf-8'";
        request.ContentLength = length;
        request.Method = "POST";
        return (HttpWebRequest)request;
    }

We take the cast to the end of the function, since it's not necessary to do it earlier.

Then we want to write the request. If you use using the stream will be closed itelf. We can change the code to this:

using (Stream requestStream = request.GetRequestStream())            
            requestStream.Write(bytes, 0, bytes.Length);

Then the function receives the response, we can take that to a different function too, like this:

 private string ReceiveResponse(HttpWebRequest request)
    {            
        using (HttpWebResponse response = (HttpWebResponse)request.GetResponse())                          
            return new StreamReader(response.GetResponseStream()).ReadToEnd();             
    }

In this function we send Data and then we get the response, so the mame must be changed to something like SendDataAndGetResponse.

Now we go back to the top, and we see that we change some things that we had written before, like instead of using a string[] we can use a List of the class itelf.(I'm tired from explaining everything, so I just post the final code here):

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

    namespace test
{
    internal class Program
    {
        private static void Main(string[] args)
        {
            ServicePointManager.DefaultConnectionLimit = 10000;
            string serverURL;
            List<TestClass> fileNames = new List<TestClass>();

            Console.WriteLine("Please enter the URL to send the XML File");
            serverURL = Console.ReadLine();

            string currentLine = "";
            while (currentLine != "start")
                fileNames.Add(new TestClass(currentLine));
            foreach (TestClass tClass in fileNames)
                tClass.SendDataAndGetResponseAsync();
            Console.ReadLine();
        }
    }

    internal class TestClass
    {
        private string xmlPath;

        public TestClass(string _xmlPath)
        {
            xmlPath = _xmlPath;
        }

        public void SendDataAndGetResponseAsync()
            => new Task(SendDataAndGetResponse).Start();

        public void SendDataAndGetResponse()
        {
            try
            {
                ServicePointManager.DefaultConnectionLimit = 10000;
                byte[] bytes = File.ReadAllBytes(xmlPath);
                HttpWebRequest request = CreateRequest(bytes.Length);
                using (Stream requestStream = request.GetRequestStream())
                    requestStream.Write(bytes, 0, bytes.Length);
                Console.WriteLine(ReceiveResponse(request));
            }
            catch (Exception ex)
            {
                Console.WriteLine(ex.Message);
            }
        }

        private HttpWebRequest CreateRequest(int length)
        {
            WebRequest request = WebRequest.Create(ATMSimulator.Properties.Settings.Default.KTHBaseUri);
            request.ContentType = "text/xml; encoding='utf-8'";
            request.ContentLength = length;
            request.Method = "POST";
            return (HttpWebRequest)request;
        }

           private string ReceiveResponse(HttpWebRequest request)
        {
            using (HttpWebResponse response = (HttpWebResponse)request.GetResponse())
                return new    StreamReader(response.GetResponseStream()).ReadToEnd();
            }
       }
   }

I hope this helps you :). I had some free time so I wrote the long answer.

Answer 2

Firstly, congrats on trying to get a better handle on these concepts. Properly organized code is an art-form and multi-threaded software (particularly debugging it) is wizardry that few understand and even fewer do well (myself included).

That being said, there are a couple of things I would recommend you tweak in your code.

1 -- Avoid using the Thread type directly if possible

---

The short story behind this is as I mentioned above. Working with threading in a modern, managed language like .NET is not for the faint of heart and can lead to excruciatingly difficult to diagnose bugs (not the least of which are race conditions, invalid object handles, and memory leaks just to name a few). Microsoft understands this complexity and to help combat, they have created many higher level abstractions that tend to help avoid some of these more common problems. You can read about the abstractions here: https://msdn.microsoft.com/en-us/library/hh156548(v=vs.110).aspx

In this case, I would suggest you use the TPL or (Task Parallel Library).

Here is a direct quote from the article on the TPL:

The Task Parallel Library (TPL) is based on the concept of a task, which represents an asynchronous operation. In some ways, a task resembles a thread or ThreadPool work item, but at a higher level of abstraction. The term task parallelism refers to one or more independent tasks running concurrently. Tasks provide two primary benefits:

• More efficient and more scalable use of system resources.

• More programmatic control than is possible with a thread or work item.

For both of these reasons, in the .NET Framework, TPL is the preferred API for writing multi-threaded, asynchronous, and parallel code.

Please have a look here for some really good information on idea behind the TPL and how to implement it: https://msdn.microsoft.com/en-us/library/dd537609(v=vs.110).aspx

2 -- Prefer usings over manual closing of unmanaged resources

---

The code here:

// Get response from Webserver
using (HttpWebResponse response = (HttpWebResponse)request.GetResponse()) {
    Stream responseStream = response.GetResponseStream();
    string responseStr = new StreamReader(responseStream).ReadToEnd();
    Console.WriteLine(responseStr + Environment.NewLine);
    responseStream.Close();
}

Can be re-written:

string responseStr = string.Empty;

// Get response from Webserver
using (HttpWebResponse response = (HttpWebResponse)request.GetResponse()) {
    using (var responseStream = new StreamReader(response.GetResponseStream())) {
        responseStr = responseStream.ReadToEnd();
    }
}

Console.WriteLine("{0}{1}", responseStr, Environment.NewLine);

This provides several benefits:

1. You can / should want to close the stream ASAP. So doing things like writing to the console and allocating variables are things that can be done elsewhere.
2. The nested using is the preferred "best-practice"

Further reading:

• https://stackoverflow.com/questions/1034639/which-is-better-and-when-using-statement-or-calling-dispose-on-an-idisposabl
• https://msdn.microsoft.com/en-us/library/yh598w02.aspx
• https://stackoverflow.com/questions/10984336/using-block-vs-idisposabe-dispose

3 -- SOLID

---

Implementing S.O.L.I.D. coding practices in your design will dramatically improve the re-usability, test-ability, and flexibility of your code. In particular, making sure each separate "task" has a well-defined encapsulation that separates that work from other work. In my final example, I will illustrate my application of some of these principles.

For more info on S.O.L.I.D., please see some of the following resources:

• http://www.blackwasp.co.uk/SOLIDPrinciples.aspx
• http://butunclebob.com/ArticleS.UncleBob.PrinciplesOfOod

Example Code

using System;
using System.Collections.Generic;
using System.IO;
using System.Linq;
using System.Net;
using System.Text;
using System.Threading;
using System.Threading.Tasks;
using System.Xml;

namespace XMLSender {
    public class Program {
        public static void Main(string[] args) {
            string targetServer = string.Empty;
            List<Task> tasks = new List<Task>();
            Collection<XmlUploadRequest> requests = new Collection<XmlUploadRequest>();
            
            targetServer = LoadTargetServer();
            requests = PromptForInputFiles();

            // Apply the target server to all requests:
            requests.ToList().ForEach(r => r.TargetServer = targetServer);

            // Build a task for each request
            foreach (var request in requests) {
                Task newTask = new Task((x) => UploadWorker.ExecuteWork(x as XmlUploadRequest), request);
                tasks.Add(newTask);
            }
            
            // the call to .Start will schedule the task using the DefaultTaskScheduler instance
            foreach (var task in tasks) {
                task.Start();
            }

            Task.WaitAll(tasks.ToArray()); // Wait for all tasks to complete.
            
            Console.WriteLine("Requests complete.");
            Console.ReadLine();
        }

        internal static string LoadTargetServer() {
            Console.WriteLine("Please enter the URL to send the XML File");
            return Console.ReadLine();
        }

        internal static Collection<XmlUploadRequest> PromptForInputFiles() {
            List<string> xmlFiles = new List<string>();
            string currentFile = string.Empty;
            
            do {
                Console.WriteLine("Please enter the XML File you Wish to send, To start simulation type 'start'");
                currentFile = Console.ReadLine();
                if (currentFile != "start") {
                    xmlFiles.Add(currentFile);
                }
            }
            while (currentFile != "start");
            
            Collection<XmlUploadRequest> requests = BuildRequests(xmlFiles.ToArray());
            
            return requests;
        }

        internal static Collection<XmlUploadRequest> BuildRequests(string[] xmlFiles) {
            Collection<XmlUploadRequest> requests = new Collection<UserQuery.XmlUploadRequest>();
            FileProvider provider = new FileProvider();
            if (xmlFiles == null || !xmlFiles.Any()) {
                throw new ArgumentException("No xml files provided.", nameof(xmlFiles));
            }

            foreach (var file in xmlFiles) {
                try {
                    string contents = provider.LoadFileContents(file);

                    XmlUploadRequest request = new XmlUploadRequest() {
                        HttpAction = "POST",
                        UploadContents = contents
                    };
                } catch (FileLoadException fle) {
                     // Catch our custom type and continue attempting to load the files in order.
                     Console.WriteLine("{0}: {1}", fle.Message, fle.FileName);
                }
            }
            
            return requests;
        }
    }

    public static class UploadWorker {

        static UploadWorker() {
            // Set the connection limit of HTTP
            System.Net.ServicePointManager.DefaultConnectionLimit = 10000;
        }

        public static void ExecuteWork(XmlUploadRequest xmlRequest) {
            HttpWebRequest request = (HttpWebRequest)WebRequest.Create(xmlRequest.TargetServer);
            int contentLength = xmlRequest.ContentLength();
            
            request.ContentType = "text/xml; encoding=utf-8";
            request.ContentLength = contentLength;
            request.Method = xmlRequest.HttpAction;
            request.KeepAlive = false;

            // Closing of the request stream isn't necessary because the using will handle the safe disposal of the unmanaged resources
            using (var stream = request.GetRequestStream()) {
                stream.Write(System.Text.Encoding.UTF8.GetBytes(xmlRequest.UploadContents), 0, contentLength);
            }

            string responseStr = string.Empty;

            // Get response from Webserver
            using (HttpWebResponse response = (HttpWebResponse)request.GetResponse()) {
                using (var responseStream = new StreamReader(response.GetResponseStream())) {
                    responseStr = responseStream.ReadToEnd();
                }
            }

            Console.WriteLine("{0}{1}", responseStr, Environment.NewLine);
        }
    }

    /// <summary>
    /// A type used to pass request data and customized request settings for an XML file upload.
    /// This type can be extended or have additional properties added to it to support customizing
    /// various aspects of an HTTP request, such as custom headers or request timeout.
    /// </summary>
    public class XmlUploadRequest {

        public XmlUploadRequest() {
            this.UploadContents = "<content>empty</content>"; // meaningful defaults
            this.HttpAction = "POST"; // meaningful defaults
            this.TargetServer = "http://test.com"; // meaningful defaults
        }

        public virtual int ContentLength() {
            return System.Text.Encoding.UTF8.GetBytes(this.UploadContents).GetLength(0);
        }

        public string UploadContents { get; set; }

        public string HttpAction { get; set; }

        public string TargetServer { get; set; }
    }

    public class FileProvider {
        
        private Encoding _fileEncoding;
        
        public FileProvider() : this(Encoding.UTF8) {
        }

        public FileProvider(Encoding enc) {
            this._fileEncoding = enc;
        }

        public string LoadFileContents(string filePath) {
            if (string.IsNullOrWhiteSpace(filePath)) {
                throw new ArgumentNullException(nameof(filePath), "No file specified to load.");
            }

            // Note, this is a "best-effort" check, we know that the nature of the file system is that the file could be present when the call to exists happens and not when we go to read (or vice-versa).
            // We still want a pre-emtive check
            if (!File.Exists(filePath)) {
                throw new FileLoadException("Unable to find the XML file specified.", filePath);
            }
            
            string returnValue = string.Empty;

            try {
                returnValue = File.ReadAllText(filePath);
            } catch (IOException ioe) {
                Console.WriteLine("Unable to read the file {0}.\n\nReason:{1}\n{2}", filePath, ioe.Message, ioe.StackTrace);
            }
            
            return returnValue;        
        }

    } // end class FileProvider

    public class FileLoadException : FileNotFoundException {

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

        public FileLoadException(string message, string fileName) : base(message, fileName) {
        }

        public FileLoadException(string message, Exception innerException) : base(message, innerException) {
        }

        public FileLoadException(string message, string fileName, Exception innerException) : base(message, fileName, innerException) {
        }
    }
}

Example Explanation

1 Program class changes

I broke your Program class out into several encapsulated static methods that each have a very specific sub-task they perform. This modular design has several benefits:

1. It breaks the code up into logic blocks that are a bit easier to read.
2. The methods themselves can be tested individually
3. The design allows for the encapsulated methods to be called from various places as the program increases in complexity.
4. It helps to isolate change and reduce code-duplication

Your Program class now also has but one responsibility: Gather user input. All other responsibilities (such as making HTTP calls, file IO, etc.) are all handled elsewhere.

I have also change the Program class to use the TPL instead of using threads directly. In this way, we don't need to worry about potential issues with joining and debugging is much nicer.

2 'FileLoadException' type

I introduced a FileLoadException custom exception type that derives from the System FileNotFoundException type. I did this mostly to demonstrate isolated exception handling and to allow for possible extension of the type for use in other ways (such as detecting whether or not an XML file is valid, etc...)

If the program encounters this type, it will simply continue processing the other specified files, but any other exception type will be un-handled and force the application to exit.

For more on how to properly handle exceptions, Eric Lippert has some EXCELLENT stuff: https://blogs.msdn.microsoft.com/ericlippert/2008/09/10/vexing-exceptions/

3 SRP and the XmlUploadRequest type

Separating concerns is in my opinion the HARDEST part of good OO programming. Many would say that naming things is the hardest part, but I say that naming things is a function of defining their responsibility and separating the concerns.

In this case, I opted to create the XmlUploadRequest type to be a "Unit-Of-Work" model that has all of the information the UploadWorker type needs to perform it's job. The XmlUploadRequest is also flexible (as is the UploadWorker) because we have isolated the responsibility of how / where to load content. It could be in the future that you want to load XML information from a database. If that were to happen, you would need to modify exactly 0% of the code in the UploadWorker or the XmlUploadRequest.

One of the benefits of the XmlUploadRequest type is that it is simple, doesn't have any complex logic and is only concerned about what the class name indicates it is concerned with:

What data is needed to upload XML?

As such, the only modifications that need to happen (see the Open Closed Principle) are those that fix bugs. But you could create an ExtendedTimeoutXmlUploadRequest type that inherits from XmlUploadRequest that specifies additional information that can be used by the UploadWorker type.

This feature means your code-base is STABLE because you can trust that the existing code will not be modified as a regular work item. This means as a developer you spend less time fighting fires and more time adding features.

4 SRP and the FileProvider type

Normally for something like this I would actually create an interface like IDataProvider and use that in the static BuildRequests method in the Program class. That way I could either give it a DatabaseProvider or a FileProvider but for brevity and simplicity, I left that out (if you like, I can include more detail regarding this).

This class' sole responsibility is to load XML data from the file system. Also notice that I load the data up-front, so there are fewer reasons for the Task to fail. By front-loading the parsing of XML files, I can perform the "mise en place" of programming and separate operations into logical "batches". This is a key feature of OO programming that allows common functionality to be grouped. In your initial example (which worked fine), the upload process did file at a time. If the file is deleted, or the upload takes abnormally long, it's more efficient to be able to immediately begin processing the next item as well as cleanly separates the responsibilities of the code, thereby making them more modular.

Miscellaneous items

Overall everything you had wasn't bad at all and there are near INFINITE ways in which you can "organize" your types. In the end it all comes down to what your intentions are, who else will work on / maintain the code, how long the code will be in production, and many other factors.

Setting the ServicePointManager.DefaultConnectionLimit is to set a property on a static instance that affects any and all threads in the current AppDomain. I moved this call to a shared constructor, but you could put it anywhere you want as long as you understand that it only needs to be called once. In your example you were calling it for every request. While this isn't necessarily a problem, it is definitely unnecessary and not the expected usage of the property.

• https://msdn.microsoft.com/en-us/library/system.net.servicepointmanager.defaultconnectionlimit(v=vs.110).aspx
• https://msdn.microsoft.com/en-us/library/system.net.servicepointmanager(v=vs.110).aspx