views:

93

answers:

2

I created an application which has to periodically perform some tasks using threads. I am not sure if this is the best way for doing it so I would really appreciate if anyone could suggest a better way.

This is the way I did it:

This is the class which includes the functions for a continuous process (looking for inactive sessions and cleaning idle items):

public class SessionCleaner
{
    private SQLWorks sqlWorks;
    public SessionCleaner()
    {
        sqlWorks = new SQLWorks();
    }

    private void cleanIdleSessions()
    {
        //this function deletes sessions from database
        sqlWorks.CleanIdleSessions(10);
    }



    //this is an endless loop for executing the cleaning every 5 seconds
    public void DoCleanIdleSessions()
    {
        while(true)
        {
            cleanIdleSessions();
            Thread.Sleep(5000);
        }
    }
}

This is the main form where the thread is initialized:

public partial class FormMain : Form
{

...
public FormMain()
    {
        InitializeComponent();
...
        startSessionCleaner();
...
    }

private void startSessionCleaner()
    {
        initializeSessionCleanerThread();
        sessionCleanerThread.Start();
    }

private void initializeSessionCleanerThread()
    {
        sessionCleaner = new SessionCleaner();
        sessionCleanerThread = new Thread(new ThreadStart(sessionCleaner.DoCleanIdleSessions));
    }

private void terminateSessionCleanerThread()
    {
        try
        {
            sessionCleanerThread.Join(1000);
        }
        catch(Exception ex)
        {
            string sDummy = ex.ToString();
        }
    }

private void FormMain_FormClosing(object sender, FormClosingEventArgs e)
    {
        terminateSessionCleanerThread();
    }

Thanks!

+2  A: 

The biggest problem I can see is... why would it ever exit? You do a Join, so obviously expect it to terminate, but is is just while(true). I would have a (volatile) bool field somewhere (on the SessionCleaner) that gets set/cleared and used in the while - for example:

volatile bool keepRunning = true;

or similar (set it to false before calling Join to exit).

I also don't see much point in keeping the reference to sessionCleanerThread (just initialize and start it in one method), and the exception swallowing is probably a bad idea.

Marc Gravell
A: 

The terminateSessionCleanerThread() method will never return, apparently, as noted by Marc. Unless there is something we cannot see.

There's a related question regarding using new Thread() versus ThreadPool.QueueUserWorkItem(). In my guess, I think this application wants to use QUWI. No reason not to do so. new Thread() will create a foreground thread. That will not matter if the thread terminates, but it isn't necessary for this app.

Does the clean really need to run every 5 seconds? Seems like that would make sense on a server, under high load. But in that case a WinForms app seems like the wrong tool for the job. Should be a Windows Service that logs messages to the event log, etc.

Cheeso