views:

56

answers:

2

I have an ASP.NET site that I am maintaining. Currently it has code that on first access (and other times too) goes and does a bunch of data caching by writing it to files. Currently it has no locking so we cna get problems with multiple people accessing the site and multiple threads trying to write to the files at the same time. This is obviously bad so I am going to implement some locking.

My current modified code just puts a simple lock around a section of code so that later requests just wait until the first request is done.

My concern is that I haven't used locks much so I just want to check if there is any situation in which that lock could not get released? For example I have no idea what happens if that first thread is killed (eg the web server decides its run too long and shuts it down). Does the lock get automatically freed at that point? Is there anything else I need to think about while doing this?

Edit to add: Here is what I think are relevant bits of Code in case there is anything I am doing stupid...

I am using private lock objects accessed through a dictionary and don't do anything much more than wrap some code in a SyncLock statement (equivalent of C# lock).

Public Shared Sub CheckAllVariables(ByVal SourceName As String, ByVal cn As HttpContext)
    ...
    Do While dr.Read
        projectID = dr.GetInt32(dr.GetOrdinal("ProjectID"))
        Dim cacheLockObject As Object = GetCacheLockObject(projectID)
        SyncLock (cacheLockObject)
            srcName = String.Format("PROJECT_{0}", projectID)
            If cacheCon.CheckNeeded(srcName) Then
                RunFullCache(projectID, cn, Nothing)
                CheckDerivedVariables(projectID, cn)
                CheckHierarchyVariables(projectID, cn)
                cn.Session(String.Format("DerivedChecked_{0}", projectID)) = True
                projectNames.Add(srcName)
                cacheCon.CheckNeeded(srcName) = False
            End If
        End SyncLock
    Loop
    ...
End Sub


Private Shared CacheLockObjects As New Dictionary(Of Integer, Object)
Public Shared Function GetCacheLockObject(ByVal projectid As Integer) As Object
    If Not CacheLockObjects.ContainsKey(projectid) Then
        CacheLockObjects(projectid) = New Object()
    End If
    Return CacheLockObjects(projectid)
End Function

Should I wrap access to the GetCacheLockObject function in a lock to prevent the possibility of two threads going in simultaneously and both finding the cache lock doesn't exist and then both creating it and returning different ones? I'm a little unused to having to consider thread safeness (assumign I am using that term correctly).

+3  A: 

since lock is just Monitor calls in try { } finally { } it can't get stuck. The application can get stuck if it gets into deadlock state. So you should be careful while locking on several objects.

BTW, consider using database. It handles all those tasks and you will not worry about locks and writing to file. Plus a lot of other features.

Andrey
Unfortunately this is not my code. I'm in the position that somebody else writes and maintains it (badly IMHO) and I'm just trying to put an occasional tweak in to make it run a bit more smoothly.Thanks for the confirmation though. I hadn't really thought about the fact it uses the monitor. I'll dig into that code a bit to see what it does. :)
Chris
+2  A: 

You will need a separate lock for accessing and using CacheLockObjects dictionary, if you are going to be adding ProjectID's elsewhere while CheckAllVariables is being ran.

You aren't calling any functionality inside the cacheLockObject that is calling other locking mechanisms. So it doesn't look like hard locking is possible. I agree that you shouldn't have any problems with locking if the thread gets aborted mid-process.

TamusJRoyce
I feel your pain. I have a project where my lead programmer left because of how bad things got. He did everything in the application. And what he didn't do required for each loops to the database. I ended up writing my own thread locker at http://nohardlockrwlocker.codeplex.com/ to prevent all the hard locks and enumeration while modification issues I was having.
TamusJRoyce
+1, I agree, the if statement in the lookup to see if there is a syncLockObject needs to be wrapped with a `SyncLock(cacheLookupLockObject)`
Scott Chamberlain
Yeah, I realised the problem with CacheLockObjects access while doing a code review on my code since asking this question. Its good to have it explicitly stated on the question though for others who might find it. :) Cheers for the feedback. :)
Chris