11
\$\begingroup\$

Summary: Please let me know, what weaknesses do you see in the described workaround for event handlers preventing garbage collection of registered listener objects...

I'm in the development of and API for a set of devices communicating with each other, and optionally with a PC trough a socket based gateway. The API describes the multitude of different devices, channels as a class hierarchy, and also implements some kind of caching for the derived objects.

I'd like to make the API really simple to use for the developers, so I keep the interfaces as simple as possible: hiding background threading, etc... I ran into the already described problems, when registering event handlers from a windows form interface:

  1. The event source object is in the cache, it keeps the reference to the target object, preventing it from garbage collection. It's quite problematic, because the UI creates and destroys controls (providing some kind of user interface for the device objects) on the fly... for example when a new device is discovered, or when view is changed.

  2. As the event raiser might be a different thread than the UI, so it requires checking in each of the event handler routines.

I was reading the WeakEvent pattern guidelines - I'm not too happy with that solution: - It's not "standard" - so the programmer has to register for events "manually" - And quite complex to build.

I ended up with Custom Event declarations, like:

Private EHL_Received As New PacketEventHandlerList("PacketReceived")
Public Custom Event PacketReceived As EventHandler Implements I_PacketEventRaiser.PacketReceived
    AddHandler(ByVal value As EventHandler)
        EHL_Received.Add(value)
    End AddHandler
    RemoveHandler(ByVal value As EventHandler)
        EHL_Received.Remove(value)
    End RemoveHandler
        RaiseEvent(ByVal sender As Object, ByVal e As PacketEventArgs)
        EHL_Received.Invoke(sender, e)
    End RaiseEvent
End Event

Then, in the "event handler" list, instead of storing real EventHandlers, I collect custom objects: they keep weak references to the targets.

Friend Class PacketEventHandler

    Private targetRef As WeakReference = Nothing
    Private IsStatic As Boolean = False
    Friend IsValid As Boolean = True
    Friend Method As System.Reflection.MethodInfo

    Public Sub New(ByVal h As EventHandler)
        IsStatic = IsNothing(h.Target)
        If Not IsStatic Then targetRef = New WeakReference(h.Target)
        Me.Method = h.Method
    End Sub

    Public ReadOnly Property Target
        Get
            If Not IsNothing(targetRef) Then Return targetRef.Target
            Return Nothing
        End Get
    End Property

    Public Sub Invoke(ByVal sender As Object, ByVal e As EventArgs)

        ' If method is static, there is not too much to do
        If IsStatic Then
            Me.Method.Invoke(Nothing, {sender, e})
            Exit Sub
        End If

        ' If the reference is invalid, then the object was probably collectd. Mark handler as invalid
        Dim MyTarget As Object = Me.Target
        If IsNothing(MyTarget) Then
            Me.IsValid = False
            Exit Sub
        Else
            ' As we are in a different thread, do the invoke stuff if needed
            If TypeOf (MyTarget) Is Windows.Forms.Control Then
                Dim ctlTarget As Windows.Forms.Control = MyTarget
                If ctlTarget.InvokeRequired Then
                    ctlTarget.Invoke(New System.Windows.Forms.MethodInvoker(Sub() Me.Invoke(sender, e)))
                    MyTarget = Nothing
                    Exit Sub
                End If
            End If

            ' For other objects it's simple
            Me.Method.Invoke(MyTarget, {sender, e})
            MyTarget = Nothing
        End If
    End Sub
End Class

Note, that each handler object holds reference to one single handler, so a list is created.

Friend Class PacketEventHandlerList
    Private Handlers As New List(Of PacketEventHandler)
    Private Name As String

    Public Sub New(ByVal Name As String)
        ' Just keep the name for logging purposes
        Me.Name = Name
    End Sub

    Friend Sub Add(ByVal value As EventHandler)
        Console.WriteLine("{0} registers {1}", value.Target.ToString, Me.Name)
        Handlers.Add(New PacketEventHandler(value))
    End Sub

    Friend Sub Remove(ByVal value As EventHandler)
        Console.WriteLine("{0} unregisters {1}", value.Target.ToString, Me.Name)
        For I As Integer = Handlers.Count - 1 To 0 Step -1
            Dim MyEvh As PacketEventHandler = Handlers(I)
            If Object.ReferenceEquals(MyEvh.Target, value.Target) And value.Method.Equals(value.Method) Then
                Handlers.RemoveAt(I)
                Console.WriteLine("Successful remove!")
            End If
        Next
    End Sub

    Friend Sub Invoke(ByVal sender As Object, ByVal e As EventArgs)
        Dim InvalidHandlers As Integer = 0

        ' Go trough all registered handlers, and invoke them
        For Each EVH As PacketEventHandler In Handlers
            Try
                EVH.Invoke(sender, e)
            Catch ex As Exception
                LogException(ex)
            End Try
            If Not EVH.IsValid Then InvalidHandlers += 1
        Next

        ' If there were invalid handlers, just clean them up
        If InvalidHandlers > 0 Then
            For I As Integer = Handlers.Count - 1 To 0 Step -1
                If Not Handlers(I).IsValid Then Handlers.RemoveAt(I)
            Next
        End If
    End Sub

End Class

And, it seems to work.

Minor issue is, that I could't check the RemoveHandler part, as it's not called for some reasons. I'm not sure if I need that at all...

Do you see any problems with this implementation?

\$\endgroup\$

migrated from programmers.stackexchange.com Feb 8 '12 at 11:26

This question came from our site for professionals, academics, and students working within the systems development life cycle.

  • \$\begingroup\$ This may be more suitable on StackOverflow (or CodeReview?). Please don't cross-post it though - it will get automigrated if enough people agree with this and votes to close here. \$\endgroup\$ – Péter Török Feb 8 '12 at 10:51
  • \$\begingroup\$ The solution proposed seems similar to Udi Dahan's Domain Events pattern: udidahan.com/2009/06/14/domain-events-salvation personally I think Udi's solution is easier to test and doesn't have any magic strings like "PacketReceived" and doesn't require any reflection. \$\endgroup\$ – Kane Jun 22 '12 at 1:43
3
\$\begingroup\$

At first glance, PacketEventHandler is a rather confusing name for a class, but then looking at it more thoroughly I see that it's actually a wrapper around an event handler, so I see what made you call it that way, but I'd still try harder to find a better name... although I have none to suggest right now (naming is one of the hardest things to do properly!).

The way you're clearing your InvalidHandlers looks awkward.

' If there were invalid handlers, just clean them up
If InvalidHandlers > 0 Then
    For I As Integer = Handlers.Count - 1 To 0 Step -1
        If Not Handlers(I).IsValid Then Handlers.RemoveAt(I)
    Next
End If

Handlers being a List(Of PacketEventHandler), I'm pretty sure you could simply do this:

Dim Handler As PacketEventHandler
If InvalidHandlers > 0 Then
    For Each Handler In Handlers
        If Not Handler.IsValid Then Handlers.Remove(Handler)
    Next
End If

And reviewing this snippet made me wonder, are you using Option Explicit? I don't see your I declared anywhere - always use Option Explicit. The C# programmer in me would also advise you to use Option Strict just to be on a [type-]safe side (you don't have to specify explicit if you specify strict - the latter implies the former).

This is by no means a full review - but it's all I could see on a first pass. The rest roughly looks good to me.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.