Executing an SQL command on a number of files

Question

Is this thread safe?

I have a program that executes an SQL command on a number of files (selected by the user).

Here is where I create the threads:

var allThreads = new List<Task<bool>>();
if (fileList != null)
{

    foreach (var file in fileList)
    {
        var fileName = Path.GetFileName(file);
        if (File.Exists(file))
        {
            Log.AddToLog("Beginning to Process File: " + fileName);
            if (!ValidateFile(file))
            {
                Log.AddToLog(String.Format("Error! {0} is not a valid file for the {1}. Skipping this file.", fileName, JsonSettingsList.NameId));
                IsErrorsInLog = true;
            }
            else
            {
                var fileToProcess = file; //need to make copy for threading loop, otherwise it acts screwy
                var newTask = Task.Factory.StartNew<bool>(() => ProcessFile(fileToProcess));
                allThreads.Add(newTask);
            }
        }
        else
        {
            Log.AddToLog(String.Format(CultureInfo.CurrentCulture, "File {0} Does not Exist. File not processed.", fileName));
            IsErrorsInLog = true;
        }
    }


    //check result, if any of them return true, then there are errors in the log, and the user needs to be alerted.
    foreach (var task in allThreads)
    {
        try
        {
            var didThreadFail = task.Result;
            if (!IsErrorsInLog)
            {
                IsErrorsInLog = didThreadFail;
            }
        }
        catch (AggregateException aggEx)
        {
            foreach (Exception ex in aggEx.InnerExceptions)
            {
                Log.AddToLog(string.Format("Caught exception '{0}'",
                    ex.Message));
                IsErrorsInLog = true;
            }
        }
        finally
        {
            task.Dispose();
        }
    }
}

and then here is the code where I am actually accessing the database, inside of the ProcessFile() function:

//initialize oracle connection 
oraConnection.Open();
using (var command = new OracleCommand(sqlText, oraConnection))
{
    command.Transaction = command.Connection.BeginTransaction();
    foreach (var line in fileByLine.Where((i, index) => index > 0))
    {
        if (!String.IsNullOrWhiteSpace(line))
        {
            //add parameters
            command.ExecuteNonQuery();
        }
    }

    command.Transaction.Commit();
    Log.AddToLog("Successfully Processed " + filePath);
}

oraConnection.Close();

The comments are where I have omitted unnecessary code for the sake of simplicity.

Answer 1

if (fileList != null)

You can reduce nesting (which is usually a good thing) by turning this into

  if (fileList == null) { return; }

The same applies to

if (File.Exists(file))

It's better to have

if (!File.Exists(file)) { continue; }

But even better then this would be to filter the list before you start processing it:

fileList.Where(File.Exists)

Checking the fileList for null is an indicator that some other part of the code might be implemented in a wrong way_. In C# we prefer to have an empty list rather then a null.

Parallel.ForEach

Have you already tried the Parallel.ForEach? It would greatly simplify your code:

Parallel.ForEach(fileList.Where(File.Exists), fileName => 
{
    // process the file
});

This method has a lot of overloads so you'll probably find something that suits your needs.

With one of them you could for example create a connection for each parallel loop and dispose it at the end of the processing.

Here's a bigger example that might work for you. This will use the same connection for several files.

try
{
    var parallelOptions = new ParallelOptions
    {
#if DEBUG
        MaxDegreeOfParallelism = 1
#else
        MaxDegreeOfParallelism = Environment.ProcessorCount
#endif
    };

    var parallelLoopResult = Parallel.ForEach
    (
        source: fileList.Where(File.Exists),
        parallelOptions: parallelOptions,
        localInit: () => new
        {
            Connection = /* init the oracle connection*/                
        },
        body: (fileName, loopState, i, local) =>
        {
            // process the file
            // use the local.Connection
            return local; // pass the connection to the next loop
        },
        localFinally: local =>
        {
            local.Connection.Dispose();             
        }
    );
}
catch (AggregateException ex)
{
    // handle exceptions
}

See this question on SO, it compares your solution with Parallel.ForEach:

• Parallel.ForEach vs Task.Factory.StartNew