classes and threading

Question

I have the following code:

public class Search
{
    StringBuilder sb = new StringBuilder();
    string[] myparams;

    public void Start()
    {
        //Start search threads
        for (int i = 0; i < 50; i++)
        {
            tasks.Add(Task.Factory.StartNew(() => 
            {
                string text1 = GetFirstRequest(url, myparams);
                string text2 = GetFirstRequest(url, myparams);

            }, ct, TaskCreationOptions.LongRunning, TaskScheduler.Default));
        }
    }

    private string GetFirstRequest(string url, string[] myparams)
    {
        //Use stringbuilder to build the complete url with params
        //Use webrequest, response and stream to return the url contents
    }

    private string GetSecondRequest(string url, string[] myparams)
    {
        //Similar to GetFirstRequest
    }
}

For my main form I call:

Search search = new Search();
search.Start();

As you can see from the code above, individual threads are created. However, each thread is calling the same private functions in the Search class in order to access the url.

Is the code thread-safe? Is it better to place the private functions into a separate class and create a class for each thread?

Answer 1

Without seeing the actual code for GetFirstRequest and GetSecondRequest, we can't tell - but the fact that you've got an instance variable of type StringBuilder makes me skeptical. StringBuilder itself isn't thread-safe, and if you're modifying a single object in multiple threads I doubt that you'll get the result you want anyway.

If you're using StringBuilder to build a complete URL, why not just create that StringBuilder in each method? If you don't need to change any of the state of your object, you'll be a long way towards being thread-safe.

Also note that your method has a params parameter but could also access the params instance variable (which would need a different name anyway as params is a keyword in C#). Do you really need that duplication? Why not just use the instance variable from the method?

It feels like this class can be made thread-safe, but almost certainly isn't yet. You need to design it to be thread-safe - which means either avoiding any state mutation, or using appropriate locking. (The former approach is usually cleaner where it's possible.)