[2008] [WORKED AROUND] SyncLock question

[ntg]

I have the following two clases:

''' <summary>
''' This class implements a list with keys that expire
''' after a certain amount of time.
''' </summary>
''' <remarks>The timed list holds keys to items stored elsewhere.</remarks>
Public Class TimedList
 
    ''' <summary>
    ''' Event raised when a key has expired.
    ''' </summary>
    ''' <param name="sender"></param>
    ''' <param name="itemKey"></param>
    ''' <remarks></remarks>
    Public Event ItemExpired(ByVal sender As TimedList, ByVal itemKey As String)
 
    ''' <summary>
    ''' List with items.
    ''' </summary>
    ''' <remarks></remarks>
    Protected m_list As SortedList(Of String, TimedItem)
 
    ''' <summary>
    ''' Our pulse timer.
    ''' </summary>
    ''' <remarks></remarks>
    Protected WithEvents m_timer As System.Timers.Timer
 
    ''' <summary>
    ''' Lock object.
    ''' </summary>
    ''' <remarks></remarks>
    Protected Shared m_lock As New Queue
 
    ''' <summary>
    ''' Default class constructor.
    ''' </summary>
    ''' <remarks></remarks>
    Public Sub New()
        m_list = New SortedList(Of String, TimedItem)
    End Sub
 
    ''' <summary>
    ''' Starts our timer.
    ''' </summary>
    ''' <param name="sweepInterval">Sweep interval in seconds.</param>
    ''' <remarks></remarks>
    Public Sub StartTimer(ByVal sweepInterval As Integer)
        m_timer = New Timers.Timer(sweepInterval * 1000)
        m_timer.AutoReset = True
        m_timer.Start()
    End Sub
 
    ''' <summary>
    ''' Stops the timer.
    ''' </summary>
    ''' <remarks></remarks>
    Public Sub StopTimer()
        SyncLock queue.Synchronized(m_lock)
            m_timer.Stop()
            m_timer.Dispose()
            m_timer = Nothing
        End SyncLock
    End Sub
 
    ''' <summary>
    ''' Adds a new key to the timed list.
    ''' </summary>
    ''' <param name="key">Key to add.</param>
    ''' <param name="expirySeconds">Seconds after which the item expires.</param>
    ''' <remarks></remarks>
    Public Sub AddItem(ByVal key As String, ByVal expirySeconds As Integer)
        SyncLock queue.Synchronized(m_lock)
            m_list.Add(key, New TimedItem(key, Now.AddSeconds(expirySeconds)))
        End SyncLock
    End Sub
 
    ''' <summary>
    ''' Removes a key from the list.
    ''' </summary>
    ''' <param name="key"></param>
    ''' <remarks></remarks>
    Public Sub RemoveItem(ByVal key As String)
        SyncLock queue.Synchronized(m_lock)
            m_list.Remove(key)
        End SyncLock
    End Sub
 
    ''' <summary>
    ''' Renews a keys' lifetime.
    ''' </summary>
    ''' <param name="key"></param>
    ''' <param name="expirySeconds"></param>
    ''' <remarks></remarks>
    Public Sub RenewItem(ByVal key As String, ByVal expirySeconds As Integer)
        SyncLock queue.Synchronized(m_lock)
            If m_list.ContainsKey(key) = False Then
                Throw New Exception("Item with key " + key + " was not found")
            End If
            m_list(key).Expiry = Now.AddSeconds(expirySeconds)
        End SyncLock
    End Sub
 
    ''' <summary>
    ''' Clears all items in the list.
    ''' </summary>
    ''' <remarks></remarks>
    Public Sub Cleanup()
        SyncLock queue.Synchronized(m_lock)
            m_list.Clear()
        End SyncLock
    End Sub
 
    ''' <summary>
    ''' Called when the timer elapses.
    ''' </summary>
    ''' <param name="sender"></param>
    ''' <param name="e"></param>
    ''' <remarks></remarks>
    Private Sub m_timer_Elapsed(ByVal sender As Object, ByVal e As System.Timers.ElapsedEventArgs) Handles m_timer.Elapsed
        m_timer.Stop()
 
        Dim remList As New List(Of String), en As IEnumerator(Of KeyValuePair(Of String, TimedItem)) = Nothing
 
        SyncLock m_lock.SyncRoot
            Try
                en = m_list.GetEnumerator()
                en.Reset()
                While en.MoveNext
                    If en.Current.Value.HasExpired Then
                        Try
                            RaiseEvent ItemExpired(Me, en.Current.Key)
                        Catch ex As Exception
                            Debug.WriteLine("An exception has occurred while notifying caller that an item has expired (" + ex.ToString + ")")
                        End Try
                        remList.Add(en.Current.Key)
                    End If
                End While
            Catch ex As Exception
                Debug.WriteLine("+=====================================================================+")
                Debug.WriteLine("TimedList exception")
                Debug.WriteLine(ex.ToString)
                Debug.WriteLine("+=====================================================================+")
            Finally
                If en IsNot Nothing Then
                    en.Dispose()
                    en = Nothing
                End If
                For Each key As String In remList
                    m_list.Remove(key)
                Next
                remList = Nothing
            End Try
        End SyncLock
 
        m_timer.Start()
 
    End Sub
 
End Class
 
''' <summary>
''' This class is used to represent a timed item.
''' </summary>
''' <remarks></remarks>
Public Class TimedItem
    Implements IComparable(Of TimedItem)
 
    ''' <summary>
    ''' Item key.
    ''' </summary>
    ''' <remarks></remarks>
    Protected m_key As String
 
    ''' <summary>
    ''' Item expiry.
    ''' </summary>
    ''' <remarks></remarks>
    Protected m_expiry As DateTime
 
    ''' <summary>
    ''' Get/set the item key.
    ''' </summary>
    ''' <value></value>
    ''' <returns></returns>
    ''' <remarks></remarks>
    Public Property Key() As String
        Get
            Return m_key
        End Get
        Set(ByVal value As String)
            m_key = value
        End Set
    End Property
 
    ''' <summary>
    ''' Get/set the item expiry.
    ''' </summary>
    ''' <value></value>
    ''' <returns></returns>
    ''' <remarks></remarks>
    Public Property Expiry() As DateTime
        Get
            Return m_expiry
        End Get
        Set(ByVal value As DateTime)
            m_expiry = value
        End Set
    End Property
 
    ''' <summary>
    ''' Default class constructor.
    ''' </summary>
    ''' <param name="key">Item key.</param>
    ''' <param name="expiry">Item expiry.</param>
    ''' <remarks></remarks>
    Public Sub New(ByVal key As String, ByVal expiry As DateTime)
        Me.Key = key
        Me.Expiry = expiry
    End Sub
 
    ''' <summary>
    ''' Returns True if the item has expired, False otherwise.
    ''' </summary>
    ''' <returns></returns>
    ''' <remarks></remarks>
    Public Function HasExpired() As Boolean
        Return (DateTime.Compare(Now, m_expiry) >= 0)
    End Function
 
    ''' <summary>
    ''' Implemented to allow for sort/search operations.
    ''' </summary>
    ''' <param name="other">Other instance of TimedItem.</param>
    ''' <returns></returns>
    ''' <remarks></remarks>
    Public Function CompareTo(ByVal other As TimedItem) As Integer Implements System.IComparable(Of TimedItem).CompareTo
        Return String.Compare(Me.Key, other.Key)
    End Function
 
End Class

The purpose of TimedList is to have a list that holds items that expire after some time. When that happens, the TimedList fires an event to request any cleanup and then removes the TimedItem from its list. The user of TimedList is bound to add and remove items to the list in a dynamic manner so there is a lock object used to make sure that sensitive operations (list addition and enumeration) are performed one at a time. The items of the TimedList are internally stored in a sorted list which has a key to each item.

The problem I have is beginning to manifest itself when I make heavy use of a TimedList (I have only one instance of it). When I throw at it items that expire after 5 seconds at the rate of 100 per second for two minutes, I get the following exception:

System.InvalidOperationException: Collection was modified after the enumerator was instantiated.
at System.ThrowHelper.ThrowInvalidOperationException(ExceptionResource resource)
at System.Collections.Generic.SortedList`2.Enumerator.MoveNext()
at RF.Various.TimedList.m_timer_Elapsed(Object sender, ElapsedEventArgs e)

I currently have code that works around the issue but I'd much prefer to eradicate the real problem. I can't seem to find anything wrong with my use of SyncLock but note that I'm locking on a different object than the sorted list I'm using. Any ideas anyone?

[jmcilhinney]

The problem is that you're locking on a different object every time, so none of your Synclock blocks is associated with any other. Each time you call Synchronized you're creating a new object. Besides that, you're not supposed to lock on the result of Synchronized. If you read the documentation for the Synchronized method you'll see that in the code example they provide they lock on the Queue's SyncRoot property, which you do correctly in only one place.

As far as I can tell, the only reason you've declared that Queue is to use it to synchronise access to the SortedList. That's not the way to go. If you want to synchronise access to the SortedList then just create a object to use for locking:

Private syncRoot As New Object

Now you lock on that syncRoot variable in EVERY SyncLock block so that they're all associated with each other and then only one of them at a time can be entered.

[ntg]

Believe it or not, that was done on purpose. After reading the docs, I was left with the understanding that the Synchronized method would guarantee single-thread access when doing anything except enumerations. That's why I've used SyncRoot for that part only.

I'll give it a go and let you know how it plays out.

[ntg]

I tried your suggestion, both with a simple and a shared object. Both times I deadlocked. This happens when the TimedList is removing expired resources (m_timer_Elapsed method) and someone tries to add call AddItem at the same millisecond.

The TimedList is part of a considerably larger project, with an instance of TimedList being used at the deepest level of the solution. The effect is quite profound since everything freezes.

I'll see if I can create a standalone application that duplicates the problem. Any ideas until then?

[ntg]

It turns out that it's not that easy to build a standalone executable that can stress the TimedList as the solution that it currently resides in.

I've fiddled around some more and during one of my tests I came up with a sorted list that had one item with a key of Nothing and a value of Nothing. As far as I know, there's no way to programmatically do that.

[ntg]

After some more review, I can't seem to find anything wrong with my implementation of your suggestion jmcilhinney.

The fact remains that at some point I ended up with an item in a sorted list that had a key equal to Nothing. I will go with my original code that works around the problem for now unless there's any other suggestion.