views:

299

answers:

4

This is my first crack at a method that is run periodically during the lifetime of my ASP.NET application to clean up expired sessions stored in my database. It seems to work pretty well, but the software engineer in me doesn't feel "right" about this code. I've been working with LINQ to SQL for a few months now, but I'm not very confident in the following code. I'm worried about a few things:

  1. Is the following code safe to run in a situation where the database is being accessed by different threads in my application? I have a decent understanding of the idea of transactions, but I want to make sure I'm using them properly.

  2. Is my query going to cause performance issues? Or is it appropriate in this case to select all of the records in this particular table? This method only runs every 15 minutes, so it's not like that query will be made over and over again in a short period of time.

  3. Is there a better way that I could do this? I have a nagging feeling that there is.

Code:

/// <summary>
/// Method, run periodically, to remove all sign in records that correspond to expired sessions.
/// </summary>
/// <param name="connectionString">Database connection string</param>
/// <returns>Number of expired sign in records removed</returns>
public static int Clean(String connectionString)
{
    MyDatabaseDataContext db = new MyDatabaseDataContext(connectionString);

    var signIns = db.SignIns.Select(x => x);
    int removeCount = 0;

    using (TransactionScope scope = new TransactionScope())
    {
        foreach (SignIn signIn in signIns)
        {
            DateTime currentTime = DateTime.Now;
            TimeSpan span = currentTime.Subtract(signIn.LastActivityTime);

            if (span.Minutes > 10)
            {
                db.SignIns.DeleteOnSubmit(signIn);
                ++removeCount;
            }
        }

        db.SubmitChanges();
        scope.Complete();
    }

    return removeCount;
}
+1  A: 

You can have a stored proc as a method on your database context. Why not write one that does what you want and then call it through your context? That way you can also make use of set logic rather than iterating over your collection. (Linq to SQL might compile this away - not sure how it deals with deletes.)

GalacticCowboy
I didn't think to do that. I'll try that with the code that Coderer suggested. Thanks!
unforgiven3
+7  A: 

This sounds like something you could easily do in a sproc. SQLServer gives you a GETDATE() method that returns the current time... I don't see why you can't just

 DELETE * FROM tblSignIns 
 WHERE LastActivityTime < DATEADD("minute", -10, GETDATE());

Wouldn't that do the same thing?

Coderer
In addition, the use of Linq puts extra overhead on the system by requiring extra communication between the DB Server and application
TAG
That's a great suggestion, I will try that route instead. Thanks!
unforgiven3
+1  A: 

One comment: you don't want TimeSpan.Minutes, you want TimeSpan.TotalMinutes. It may well be irrelevant most of the time, but it's a logical bug at least :)

Jon Skeet
Good point :-) Thanks!
unforgiven3
+1  A: 

FWIW I published a piece of sample code a little while back showing an implementation of set-based/batch updates for Linq (single statement update instead of record-by-record).

I haven't published the delete version of the same but will do soon (read: when I'm in the mood to type up another blog entry :) ). Meanwhile you can derive it from the 'update' statement version.

You'll find a description and the source code here: http://blog.huagati.com/res/index.php/2008/11/05/architecture-linq-to-sql-and-set-based-operations-update-statements/

Update: a 'delete' example can be found here: http://blog.huagati.com/res/index.php/2008/11/25/architecture-linq-to-sql-and-set-based-operations-delete-statements/

KristoferA - Huagati.com