Open Opiumtm opened 8 years ago
How would it look like? How would it be implemented?
@svick closer to the original event usage, the better Ideally CLR and C# language should support weak events as a first-class feature.
If we look at ConditionalWeakTable
, it use so called "dependent handle" hidden CLR feature to automatically remove key-value pairs from dictionary if primary object have been collected by GC. Similar approach may be used to implement weak event pattern. If CLR object have been collected by GC, it should automatically unsubscribe any weak events (to clean up event delegate list from references to weak event wrapper objects - it would be critical if weak event handler is attached to static long-lived object instance).
Without C# support it would be just static helper class like WeakEvent.Attach(object targetObj, string eventMemberName, Delegate handler)
and ideally should be supported in .NET Native toolchain to provide maximum performance, avoiding reflection overhead. For class with declared event it shouldn't be any difference as weak event manager should do all the job providing regular delegate which would be added to event delegates list and should be removed automatically when subscriber object is collected by GC.
I think, ConditionalWeakTable
should be used as an example of implementation. Event-publisher class shouldn't have hard reference to subscriber instance and event handler delegate should be transparently removed when subscriber is collected.
Related proposal to make weak event handlers a language feature: https://github.com/dotnet/roslyn/issues/101.
Searching around, I found two decent existing implementations of weak event:
The implementations seem to be fairly similar, resulting in code like:
private readonly WeakEventSource<MyEventArgs> _myEventSource = new WeakEventSource<MyEventArgs>();
public event EventHandler<MyEventArgs> MyEvent
{
add { _myEventSource.Subscribe(value); }
remove { _myEventSource.Unsubscribe(value); }
}
I don't think you can do much better without changing the language. Maybe the corefx version of weak event could look something like this, possibly even copying code from one of the two implementations.
@svick I know about these implementations. Major issue with all these implementations is a fundamental inability to automatically unsubscribe weak event proxies from event source. It is not critical for short-living objects, but become problem if you subscribe to static object instance with long lifetime.
Here is example implementation of weak event pattern in T10 library
public class WeakReference<TInstance, TSource, TEventArgs> where TInstance : class
{
public WeakReference<TInstance> Reference { get; protected set; }
public Action<TInstance, TSource, TEventArgs> EventAction { get; set; }
public Action<TInstance, WeakReference<TInstance, TSource, TEventArgs>> DetachAction { get; set; }
public WeakReference(TInstance instance) { Reference = new WeakReference<TInstance>(instance); }
public virtual void Handler(TSource source, TEventArgs eventArgs)
{
TInstance instance;
if (Reference != null && Reference.TryGetTarget(out instance))
{
EventAction?.Invoke(instance, source, eventArgs);
}
else
{
// Instance surely doesn't survive garbage collections, so passing null for it
// Don't removed unnecessary delegate parameter for backward compatibility
DetachAction?.Invoke(null, this);
}
}
}
DetachAction is required to properly unsubscribe proxy object from event source. This example is also flawed as if event isn't triggered for a long time, proxy objects wouldn't be detached and collected.
To avoid this, similar to ConditionWeakTable
approach should be used, known as ephemeron.
Unfortunately, ephemerons can not be used in .NET code directly as ConditionalWeakTable
rely on hidden CLR feature and use undocumented P/Invoke to achieve this goal.
I've been working on adding something like this... my current state is at https://gist.github.com/SamuelEnglard/7ed7a020d770fe137f7e5c19adce43b1
Using ConditionalWeakTable
I'm slowly getting something close to ephemeron
@SamuelEnglard sadly, it wouldn't be usable on .NET Native builds, because .NET Native doesn't support IL emit at runtime.
@Opiumtm in theory all the emitting I'm doing could be done at compile time since I'm really just "reinventing" the stuff the compiler does for current events
@Opiumtm What Template10 has is weak event handler (i.e. you specify whether it's weak or not when subscribing a handler to an event); the two libraries I linked to have weak events (you specify whether it's weak when declaring an event). Because of that, they don't require the user to manually specify any DetachAction, or anything like that.
I assumed you wanted weak events (because that's what you called it), not weak event handlers. You might want to clarify that in your original post and the title.
Major issue with all these implementations is a fundamental inability to automatically unsubscribe weak event proxies from event source.
My implementation doesn't use proxies, and handlers are automatically unsubscribed.
I've done some more work and updated the gist. Except for the fact that I'm doing emit at runtime I'm pretty sure what I've created is a "true" weak event
I've done some more work and updated the gist. Except for the fact that I'm doing emit at runtime I'm pretty sure what I've created is a "true" weak event
Could you show how your implementation is used? All I see is a WeakDelegate
class, so it looks like it's going to be the event handler that takes care of the "weakness", rather than the event publisher.
@thomaslevesque Good point, give me a minute and I'll upload my test
TestRunner.cs is what I've been using to test with
Actually, I misunderstood what your class was doing. I wrote the following test: https://gist.github.com/thomaslevesque/f5c86e23b145b841977896ca7aad59e1 It seems to work as expected đź‘Ť
Exactly. Pretty much just replace usage of the static members of Delegate
with my WeakDelegate
was the goal
@SamuelEnglard @thomaslevesque Most ridiculous part of this - .NET base class library haven't out-of-the box reference implementation of weak event/weak delegate feature. So, people are forced to reinvent the bicycle or use arbitrary custom implementation from nuget. Most UI frameworks reinvent weak events and include its support in lib. Template10 library, for example, have its own implementation of weak events. Weak events/weak delegates is a widely used feature still not standardized having no out-of-the-box reference implementation.
I should be able to have a speclet for this soon based on an updated version of my gist
Wanted to give an update that I moved my work to https://github.com/SamuelEnglard/weakdelegates
Ok, so the code in https://github.com/SamuelEnglard/weakdelegates should work.
API:
namespace WeakDelegates
{
// Usage is the same as System.Delegate.Combine(Delegate, Delegate);
public static T Combine<T>(T a, T b) where T : class;
// Usage is the same as System.Delegate.Remove(Delegate, Delegate);
public static T Remove<T>(T source, T value) where T : class;
}
Primary issue currently is that I haven't found a way to make the syntax nice and short like we have for regular delegates. In theory that is more of a C#/compiler issue though
I think this is an interesting idea and it might be worth pursuing.
@terrajobst what do you think?
While we're throwing things in, here's what I've had. An implementation using WeakDelegate
, and one using ConditionalWeakTable
. Thread-safe.
Having this in corefx would be nice, but it would be even better to have it directly in the language. The syntax could be something like this:
public weak event EventHandler MyEvent;
@thomaslevesque I don't think the event declaration should declare event. Instead the event handler subscription should declare it to be weak. The class that exposes the event can't possibly know if the instance that's subscribing is going to be a longer-lived object or not. That's something the subscriber knows.
@thomaslevesque I don't think the event declaration should declare event. Instead the event handler subscription should declare it to be weak. The class that exposes the event can't possibly know if the instance that's subscribing is going to be a longer-lived object or not. That's something the subscriber knows.
Well, if the subscriber is long-lived, it doesn't matter whether it's subscribed weakly or not, so the event publisher doesn't need to know about it anyway.
are there any updates on this ? :)
EDIT: I'm currently in the need of a cross platform weak event manager and stumbled upon this thread
I'm currently in the need of a cross platform weak event manager and stumbled upon this thread
@JaggerJo my implementation targets netstandard1.1, netstandard2.0, net35, net40, and net45, so it should work on almost all current platforms
Thank you, this might help a lot.
I’m working mostly in F# so I have to test if your implementation “feels right”.
Having the ability for a publisher to declare a weak event is far less useful than for a subscriber to have the ability to listen to an event weakly (though the former is far easier problem to solve). The reason for this is simply because there are vast numbers of existing framework classes that publish events that need be listened to weakly (think of all the events published by WPF controls for example), and those could never be changed without breaking all sorts of existing code.
Here's my implementation that I'm currently using for weak event listening in some of my projects: https://github.com/davidmilligan/WeakEventListener . It's pretty simple, but it gets the job done.
Here's my implementation that I'm currently using
There are plenty of those. But the entire point of this issue is to have it as a language feature. Also all implementations I've seen still leak (albeit very small objects). Some do clean up if the event fires after an object was released, but usually they rarely do
But the entire point of this issue is to have it as a language feature
No it’s not, or this issue is in the wrong repo. The related language feature request was rejected, and I doubt anything will ever come of it in the future (don’t get me wrong, I would love to see such a feature). However, that doesn’t preclude framework support for it, which, while it wouldn’t be nearly as nice and succinct as a language feature, would at least standardize an API for doing this sort of thing and eliminate the need for the proliferation of all these different implementations. Therefore promoting reusability and interoperability of code.
Also all implementations I’ve seen still leak
Of course they do, but that’s not the point. When a singular tiny object is kept alive instead of a tree of UI objects in the 100s of MB -> you have succeeded.
For the Weak Event implementation, would it be easier to make WeakEventHandler
and WeakEventHandler<TArgs>
base "types" the compiler can use as markers to change the generated IL implementation that wraps MulticastDelegate usage? I do still think there are a number of scenarios where it would be beneficial to let the subscriber identify itself as needing a weak subscription since a ViewModel attaching to a static handler may not know its GC Root has been unpinned and is thus unable to unsubscribe. This is most especially helpful when you don't control the code of the event you're subscribing to.
All that said, there's a large part of me that is actually of the opinion that "Weak Event" is an Anti-Pattern that allows for bad practice (and, given its commonality, we can see it now encouraging the continued use). Along these lines of thought I don't think we should encourage its continued use, which a language feature would very much do. The most common use-case I've seen is that some sort of UI control/view-model isn't informed that it's being removed from an object graph/render hierarchy; as a result it is unable to detach from some static/singleton/long lived event. As best as I can tell, if the UI hierarchy informed child graphs of removal then they could detach handlers and we wouldn't need such a thing. Dispose has been a great place for this in some of the projects I've worked on, both for the UI controls themselves as well as for models/view-models; the key being ensuring that it's called in all cases of removal/destruction, much as IDisposable is supposed to work.
there's a large part of me that is actually of the opinion that "Weak Event" is an Anti-Pattern that allows for bad practice
I would disagree, due to the UI argument you just mentioned...
The most common use-case I've seen is that some sort of UI control/view-model isn't informed that it's being removed from an object graph/render hierarchy
I don't see how that would be even possible, without using even more anti-patterns. It is also assuming it's about being removed from the object graph or not. Just because it's not currently being rendered, doesn't mean it shouldn't be observed.
Dispose has been a great place for this in some of the project
But Dispose is likely not called if it's being held on to.
Dispose is the anti pattern of all anti patterns, and it’s existence is due to probably the single biggest mistake/shortcoming of C# and the CLR.
What’s the point of having garbage collection if you’re going to require entire object graphs to be responsible for their own cleanup and notifying all their dependents of such.
What’s the point of having garbage collection if you’re going to require entire object graphs to be responsible for their own cleanup and notifying all their dependents of such.
Native objects, which doesn't have GC. It's quite literally the main reason for dispose (mentioned in the first two paragraphs of the doc). It would also be a very good reason against relying on dispose on UI objects to clean up event listeners, as that's not what Dispose is for.
Maybe I'm mistaken but I always considered weak references some heavy-weight custom finalizers. And as I always try to avoid relying on finalizers I tend to make every type disposable that has events:
public class SomeClassWithEvents : IDisposable
{
private EventHandler someEventHandler;
public event EventHandler SomeEvent
{
add => someEventHandler += value;
remove => someEventHandler -= value;
}
// disposing the object removes the event subscriptions:
public void Dispose() => someEventHandler = null;
}
Thus I don't even need to bother removing all the event subscriptions in the consumer code: a single Dispose
call is enough, or for short living objects they can be created in a using
block.
I prefer this approach even in WPF, which traditionally tries to avoid using the dispose pattern. But I can accept that others may have other preferences and that one can live happily with the overhead of weak events.
But as a language level feature... I don't know... it sounds like encouraging relying on finalizers rather than cleaning up everything properly. Or do I miss something?
Or do I miss something?
You can’t make classes you don’t control (e.g. WPF UI objects and other framework classes) disposable if they are not already. And even if they are, the implementation may not clean up event registrations like you suggest.
@koszeggy that only works if the event publisher has a shorter lifespan than the subscriber. It's not always the case.
@davidmilligan: Yes, WPF UI elements can be a pain. I never really understood why WPF decided to deny using the dispose pattern. A Window
should definitely be disposable as it owns unmanaged handles. It even has an internal IsDisposed
property and a private InternalDispose
method.
You can’t make classes you don’t control (e.g. WPF UI objects and other framework classes) disposable if they are not already.
You are right, still, I came up workarounds like this to make UI elements "disposable" (this specific solution works only for elements added to a Window
at some point but then provides a direct way to dispose them).
And even if they are, the implementation may not clean up event registrations like you suggest.
Of course. But I didn't talk about already existing components, which don't use weak events either. For them you need to remove their event registrations explicitly, that's why one needs the aforementioned workarounds (in WPF, at least) where you can do such cleanups.
But for newly designed components, where you can decide whether to use weak events or remove the handlers in Dispose
I tend to vote for the latter.
I never really understood why WPF decided to deny using the dispose pattern.
From https://docs.microsoft.com/en-us/dotnet/api/system.idisposable?view=netframework-4.8 :
Provides a mechanism for releasing unmanaged resources. The primary use of this interface is to release unmanaged resources. The garbage collector automatically releases the memory allocated to a managed object when that object is no longer used. However, it is not possible to predict when garbage collection will occur. Furthermore, the garbage collector has no knowledge of unmanaged resources such as window handles, or open files and streams. Use the Dispose method of this interface to explicitly release unmanaged resources in conjunction with the garbage collector. The consumer of an object can call this method when the object is no longer needed.
@dotMorten: Even an empty WPF Window
uses unmanaged resources, still, it is not disposable.
Secondly, the dispose pattern is definitely not just for unmanaged resources.
From https://docs.microsoft.com/en-us/visualstudio/code-quality/ca1063?view=vs-2019#pseudo-code-example:
NOTE: Leave out the finalizer altogether if this class doesn't own unmanaged resources, but leave the other methods exactly as they are.
where those other methods are Dispose()
and Dispose(bool disposing)
@koszeggy Your example is showing disposing other managed resources that has unmanaged resources, so my argument still stands (yes you should often be disposing IDisposable objects when it's not coming from the finalizer, but in the end it's about releasing the native resources)
@dotMorten: Well, not quite. Dispose
is simply about deterministic releasing of resources. Maybe this third MS Docs page about the dispose pattern covers the complete truth:
The body of the method consists of two blocks of code:
- A block that frees unmanaged resources. This block executes regardless of the value of the disposing parameter.
- A conditional block that frees managed resources. This block executes if the value of disposing is true. The managed resources that it frees can include:
- Managed objects that implement
IDisposable
. [...]- Managed objects that consume large amounts of memory or consume scarce resources. [...]
And maybe it's just my taste but I consider potentially non auto-reclaimable big objects scarce resources.
Btw, I like the approach of streams: similarly to WPF, neither the base Stream
itself, nor most of its derived types own any unmanaged resources, still, even the abtract base Stream
class implements the dispose pattern, which is very beneficial when we use a FileStream
, for example.
But before being too offtopic let's return to weak events. As I said I can accept if someone prefers them. And I understand that they can be tempting in environments such as WPF, which didn't make releasing resources in a deterministic way easy. Though workarounds like this can be complicated, I still think using weak events is just choosing an even worse solution.
My main concern about weak references that they just bring in more unmanaged resources to solve the problem. Even worse, WeakReference
doesn't even implement IDisposable
so you must rely on its finalizer, which ends up in an extern native call somewhere deep in the CLR where it has a special treatment. If WPF tries to keep itself away from unmanaged resources (but see Window
, again), then I would go for the managed way. Not necessarily by disposing as I originally suggested but by hooking up regular event removals even if it sometimes needs workarounds like the one I mentioned.
Final words: again, I accept if someone prefers using an architecture built up around weak events. You can find very nice weak event manager solutions everywhere on the net and that's fine. I just wouldn't be happy to see that they are encouraged to be used everywhere by getting language support.
Streams are a great example of use of native resources (sockets, file handles etc). It's more the exception than the rule that a stream is purely managed (And those that are purely managed doesn't really clean up large resources because they rely on finalizers to do that)
Anyway yes back on topic. As mentioned above Dispose
on UI Elmements doesn't really solve this problem, nor is it a valid option to suddenly throw IDisposable
on all WPF elements and require users to walk the UI Tree and dispose things manually.
@koszeggy Your example is showing disposing other managed resources that has unmanaged resources, so my argument still stands (yes you should often be disposing IDisposable objects when it's not coming from the finalizer, but in the end it's about releasing the native resources)
I understand that there was a managed->unmanaged problem that they knew needed a pattern to solve from the very beginning of managed code, which is a large part of how IDisposable
came into existence, but it absolutely is not the only usage of it now.
Firstly, it's often applied because of the readability and convenience of the using
language feature. ILogger.BeginScope
in Microsoft's own logging abstraction, for example.
Additionally, there are many official capacities directly in the framework and language which use Dispose
without any unmanaged resources involved. For example, any iterator (e.g. yield return
) state machine and any generated async
state machine that includes a finally block generates a Dispose method with non-trivial managed logic and ensures consuming foreach
and await
statements call it, though manually interacting with the return IEnumerable
or Task
requires manually disposing of these items.
Dispose, as a pattern, has evolved to a point where it is used to ensure that the composition of a component graph (not to be confused with the Component
class, but in essence the pattern that is modeled off) is symmetrically decomposed. In this way, when I create a UI library that dynamically adds and removes controls, I dispose the control I'm removing which disposes the controls inside of it. How is it exactly that WPF knows that none of the controls in its hierarchy will need to dispose of unmanaged resources? That's obviously a false assumption in its own right (as mentioned above). So, if a control DOES need to dispose of unmanaged resources, when a parent WPF container is removed, how are the children supposed to know to dispose their unmanaged resources? Currently they do not, and they rely on their finalizer (which is not the advised pattern of use and the finalizer queue is a lovely place to end up troubleshooting complicated performance issues too). I think we disagree on whether or not Dispose would be considered an anti-pattern in this context; obviously we can't introduce such a massive breaking change to an established (and already fading) framework, but the point is that weak events were necessary in part because there was no other backwards compatible solution, which wouldn't be the case if everything DID have a place to detach events when it was first implemented. What we CAN do is try not to repeat the mistake in the future, and that requires some way of knowing when a control is being destroyed (I think Dispose fits this bill well, but any option is better than no option; an OnDestroying
override or some such would still allow me to dispose unmanaged resources and detach event handlers .
Also, the recommended IDisposable
imlementation pattern identifies the concept of having differing code paths for when a finalizer is called vs. when a type is disposed explicitly, particularly because in the finalizer a variety of managed cleanup operations are not relevant (e.g. detaching an event handler, which isn't necessary in a finalizer because there are no objects in the graph which are preventing the instance from being collected). If Dispose
should not be used to cleanup managed resources, this part of the pattern simply wouldn't need to exist.
So now when I create an MVVM framework, if a parent view model is being destroyed I call Dispose
on it and it disposes of any native resources (rare) as well as any nested disposable view models; the views I've written which bind to these in turn respond to the change event removing some child and removes/disposes of the view which maps to that child. So in this case I remove a view model from a collection of child elements; that child view model has subscribed to a static event--perhaps something related to Android activities launching--this could use a weak event handler where the calling event knows nothing about whether the view model still exists or not and the handler simply remains in the list of delegates to call until the end of time or until some black box magic cleanup operation comes through and removes the handler from the list of subscribers at an uncontrolled to the consumer point after the fact (maybe the first time it tries to execute, though it's generally bad practice to remove an item from a collection while it's being enumerated; maybe by a background thread whose sole purpose is to occasionally iterate through all the weak handlers and see if they're still alive). OR I dispose the view model I'm destroying, it detaches the event handler at exactly the moment it no longer needs it--there is no race condition that causes the event to call a handler registered to a control which is no longer in the hierarchy, but which has not yet been garbage collected (this is a real thing that has happened in my experience; the bound view threw an exception because it was no longer attached to an activity)--and instead I can predict exactly how my program behaves and when. Alternatively, Xamarin can make every event a weak event that, upon attempting to invoke items in the event list it checks to see if they're still alive and if so adds them to a set of items to remove from the list after it's done enumerating them (since you enumerate events in the order they are added it would be inappropriate to iterate backwards and remove as you go; though I suppose we could make a costly event list that shifts the entire collection and adds new items at the beginning whenever a new one is added so that we could have a cheaper removal process); now every event handler in Xamarin experiences overhead that some applications will not need/benefit from at all. On top of that, unlike ever before, if I create an object whose only purpose is to attach to some event handler(s) and call into some other object(s), it now gets garbage collected unless I add that object to a list or keep a static reference simply to pin it in place; it no longer has the same life as the object to which it attached an event handler; in fact the object no longer gets collected at all once I pin it statically and it now prevents the publisher from being collected unless the weak behavior is bidirectional; do we make keywords for each direction of weakness, as well as bidirectional? How does a subscriber know when the object it has subscribed to has been collected? I use Xamarin as an example because we have an application whose UIs are entirely dynamically generated at runtime via layouts described by a server with both views and view models which need to be aware of various global changes (Navigation, Online/Offline state changes, Suspend/Resume, Incoming Call, etc.) and which adequately unsubscribes from all event handlers without using weak events anywhere. We had to go out of our way to accomplish it, and it certainly would not have been feasible to do with traditional data-bound XAML UIs, but it is possible, more efficient, and easier to diagnose.
We are now seeing very similar benefits keeping those design principles in mind from the beginning in a Blazor application (which, I'll note, advises exactly the pattern of use I describe here).
Also, as far as a weak event publisher pattern is concerned, I would strongly advise against using ConditionalWeakTable
(at least in its current state). We tried migrating away from repetitive manual logic in every event's add/remove methods (based on wrapping the individual supplied handlers in individual weak handlers, much like subscribers can do to non-weak events) to a more generic/reusable solution based on ConditionalWeakTable
and had to undo the work because of extremely significant performance degradation. I don't like the idea of making a language feature for weak events, but if it is done perhaps it would be advisable to focus first on some sort of ephemeron/weak delegate mechanism that is more efficient/CLR-integrated first and base it on that.
So, if a control DOES need to dispose of unmanaged resources, when a parent WPF container is removed, how are the children supposed to know to dispose their unmanaged resources?
I deal with this exact scenario to tear down my Direct3D rendering. That's what the Unloaded event does for you (it's fired when it's removed from the view hierarchy), or alternatively IsVisibleChanged
for also detecting collapse/visible state changes. In most cases when a UI Control isn't loaded/rendering, there's no need to be monitoring events, holding on to big expensive unmanaged resources etc.
The problem with dispose is that it's use-once. However in for-instance a tab-control scenario, the control gets loaded/unloaded multiple times when you flip back and forth between tabs. There's even guidelines for holding onto a singleton of an expensive view control, and reuse it rather than creating new ones over and over again when you navigate back and forth between pages. And then we also get into virtualized controls that are being reused over and over again for different dataitems.
Yet another WeakEvent implementation provided by: System.Waf.Core. Motivation
Example for INotifyPropertyChanged.PropertyChanged:
public class Publisher : INotifyPropertyChanged
{
public event PropertyChangedEventHandler? PropertyChanged;
}
public class Subscriber
{
public void Init(Publisher publisher)
{
// Instead of publisher.PropertyChanged += Handler; use the following statement:
WeakEvent.PropertyChanged.Add(publisher, Handler)
}
public void Handler(object? sender, PropertyChangedEventArgs e) { }
}
More details can be found on this Wiki page Weak Event.
Honestly, the weak event pattern should not only be added to the runtime but enabled by default -- for ALL events.
This is a long-standing issue with .NET on par with null reference exceptions. If they can add nullable reference types then can certainly make the events use weak references by default in the entire framework. This would eliminate a massive amount of memory leaks that really shouldn't happen so easily in 2022 in a managed language.
Honestly, the weak event pattern should not only be added to the runtime but enabled by default -- for ALL events.
I'm pretty sure this would break a lot of existing code... Weak event support would be nice, but it shouldn't be the default.
Yes default as weak would cause more problems than we're trying to solve.
"Weak event" pattern is a widely used pattern. But still, there is no out-of-the-box support for it in .NET