Another way to implement weak event handlers in vb.net

Question

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?

• Péter Török: 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.
• Kane: 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.

Answer 1

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.