views:

141

answers:

3

So I've been told what I'm doing here is wrong, but I'm not sure why.

I have a webpage that imports a CSV file with document numbers to perform an expensive operation on. I've put the expensive operation into a background thread to prevent it from blocking the application. Here's what I have in a nutshell.

protected void ButtonUpload_Click(object sender, EventArgs e)
{
    if (FileUploadCSV.HasFile)
    {
        string fileText;
        using (var sr = new StreamReader(FileUploadCSV.FileContent))
        {
            fileText = sr.ReadToEnd();
        }

        var documentNumbers = fileText.Split(new[] {',', '\n', '\r'}, StringSplitOptions.RemoveEmptyEntries);

        ThreadStart threadStart = () => AnotherClass.ExpensiveOperation(documentNumbers);
        var thread = new Thread(threadStart) {IsBackground = true};
        thread.Start();
    }
}

(obviously with some error checking & messages for users thrown in)

So my three-fold question is: a) Is this a bad idea? b) Why is this a bad idea? c) What would you do instead?

+1  A: 

a: yes.

Use the ThreadPool;) Queue a WorkItem - avoids the overhead of generating tons of threads.

TomTom
+7  A: 

I recommend you use the BackgroundWorker class instead of using threads directly. This is because BackgroundWorker is designed specifically to perform background operations for a graphical application, and (among other things) provides mechanisms to communicate updates to the user interface.

Justin Ethier
But `BackgroundWorker` uses `ThreadPool` threads, which should not be used for long-running operations.
Toby
Do you have a reference for this?
Justin Ethier
It also depends on the definition of "long-running". There's a difference between "long enough we don't want to make the user sit and wait for it" and "hours or days."
Joel Mueller
These operations are probably a few minutes, but possibly upwards of two hours.
Matt Grande
@Justin - See the end of this posting for a justification of why to use a dedicated thread instead of a pool thread for long-running tasks: http://blogs.msdn.com/pedram/archive/2007/08/05/dedicated-thread-or-a-threadpool-thread.aspx
ebpower
+8  A: 

A possible problem is that your background thread is running in your web sites application pool. IIS may decide to recycle your application pool causing the expensive operation to be killed before it is done.

I would rather go for an option where I had a separate process, possibly a windows service, that would get the expensive operation requests and perform them outside the asp.net process. Not only would this mean that your expensive operation would survive an application pool restart, but it would also simplify your web application since it didn't have to handle the processing.

Telling the service to perform the expensive process could be done using some sort of inter-process communication, the service could poll a database table or a file, or you could use a management queue that the service would listen to.

There are many ways to do this, but my main point is that you should separate the expensive process from your web application if possible.

Rune Grimstad