current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 3 down vote favorite

I wrote a script to test how long it takes to make requests against a url. It takes a list of urls, an array of concurrent requests, and a number of times to attempt the test. How could I improve on this?

//https://www.nuget.org/packages/RestSharp

string baseUrl = "http://localhost";
void Main()
{
    var urls = new string[] { 
        "/some/end/point",
    };

    var concurrentRequests = new int[]{ 1, 5, 25, 50, 100, 500, 1000};
    var numberOfTurns = 10;
    RunTests(urls, concurrentRequests, numberOfTurns);

}

private void RunTests(string[] urls, int[] concurrentRequests,  int numberOfTimes = 1)
{
    for(var i = 0; i < numberOfTimes; i++)
    {
        RunFor(urls, concurrentRequests);
    }
}
private void RunFor(string[] urls, int[] concurrentRequests)
{
    concurrentRequests.ToList().ForEach( e =>   
    {
         RunTest(urls, e);
    }); 
}
private void RunTest(string[] sites, int iterations, string description = "")
{
    var action = GetAction();

    var watch = new Stopwatch(); 
   // Construct started tasks
   Task<bool>[] tasks = new Task<bool>[sites.Count()];
   watch.Start();

   for(int j = 0; j < iterations; j++)
   {
        for (int i = 0; i < sites.Count(); i++)
        {
            tasks[i] = Task<bool>.Factory.StartNew(action, sites[i]);
        }
   }
   try
   {
       Task.WaitAll(tasks);
   }
   catch (AggregateException e)
   {
       Console.WriteLine("\nThe following exceptions have been thrown by WaitAll()");
       for (int j = 0; j < e.InnerExceptions.Count; j++)
       {
           Console.WriteLine("\n-------------------------------------------------\n{0}", e.InnerExceptions[j].ToString());
       }
   }
   finally
   {
    watch.Stop();
    Console.WriteLine("\"{0}|{1}|{2}\", ",sites.Count(), iterations, watch.Elapsed.TotalSeconds);
   }
}
private Func<object, bool>  GetAction()
{
    baseUrl = baseUrl.Trim('/');
    return (object obj) =>
   {
        var str = (string)obj;
        var client = new RestClient(baseUrl);
        client.Authenticator = new NtlmAuthenticator();
        var request = new RestRequest(str, Method.GET);
        request.AddHeader("Accept", "text/html");
        var response = client.Execute(request);     
        return (response != null);
   };
}
share|improve this question

3 Answers 3

up vote 3 down vote accepted

The default number of concurrent requests to the same server (ServicePointManager.DefaultConnectionLimit) is only 2. So I believe most of your tests are going to be limited by that and not by the server you're trying to test.

You should change the value to something larger (probably concurrentRequests.Max()).

concurrentRequests.ToList().ForEach( e =>   
{
     RunTest(urls, e);
});

I don't see any reason to use List.ForEach() here (or pretty much anywhere else). Normal foreach is simpler and more efficient (no need to copy the array to a List):

foreach (int requests in concurrentRequests)
{
     RunTest(urls, requests);
}

I have also change the name of the iteration variable to request to make the code somewhat more readable.

for(int j = 0; j < iterations; j++)
for (int i = 0; i < sites.Count(); i++)

You should be consistent regarding whether you put a space after the for keyword. The common way is to put a space there.

tasks[i] = Task<bool>.Factory.StartNew(action, sites[i]);

This can be simplified to:

tasks[i] = Task.Factory.StartNew(action, sites[i]);

Also, unless few extra allocations are important for you, I would use a lambda here to make the code more type-safe (no more casts in GetAction) and clearer (your GetAction returns a lambda, my TestUrl is a normal method):

var url = urls[i];

tasks[i] = Task.Factory.StartNew(() => TestUrl(url));

Few notes about this code:

  • I have changed sites to urls, it make it consistent between methods and also because I believe that's more correct term.
  • The url variable is important here, Task.Factory.StartNew(() => TestUrl(urls[i])) wouldn't work correctly, because the i variable is shared among iterations of the loop, while url isn't.
for(int j = 0; j < iterations; j++)
{
     for (int i = 0; i < sites.Count(); i++)
     {
         tasks[i] = Task<bool>.Factory.StartNew(action, sites[i]);
     }
}

If iterations is greater than 1, you're going to overwrite old Tasks with new ones, which means you're not going to wait for the old tasks to complete. That's not okay.

Console.WriteLine("\nThe following exceptions have been thrown by WaitAll()");

I find it confusing that you use both \n and WriteLine() for newlines. I would prefer to write it like this:

Console.WriteLine();
Console.WriteLine("The following exceptions have been thrown by WaitAll()");

An alternative if you want to be concise is:

Console.Write("\nThe following exceptions have been thrown by WaitAll()\n");
share|improve this answer
up vote 4 down vote

You're not always using var in what I think of as a good way. We should only use the keyword when it's obvious what type the variable is. Consider this line.

Task<bool>[] tasks = new Task<bool>[sites.Count()];

It's completely obvious what tasks is because you're newing it up right there on that line. There's no reason not to simplify by using var.

But here, you have the opposite problem. 

var response = client.Execute(request);

It is not obvious what the return type is, so you shouldn't be using var.

share|improve this answer
up vote 1 down vote

  • I don't see a reason, why you prefer the extension method Count() over the Length property of the string[] sites array.

  • Comments should describe why something is done and they should be valid / true 

    // Construct started tasks
Task<bool>[] tasks = new Task<bool>[sites.Count()];

    this comment is

    1. boilerplate code
    2. misleading because neither does the next line construct any task nor started tasks.

  • If this should be used by others also, you need to validate the input parameters. In the current state, if e.g sites == null you will get an ArgumentNullException.

  • the optional parameter description can be removed, because it isn't used at all

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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