Provide simple syntax to create a weak-referenced eventhandler

[dotMorten]

The by far most common memory leak people introduce in .NET apps are caused by event handlers added to longer-lived objects. For instance an event on a static instance, or on an instance who's lifetime is the entire app lifetime (like app view model or app settings).
Listening for events on this from a shorter-lived object, makes this short-lived object hang around in memory until the app stops running. A common scenario is the INotifyCollectionChanged event listened to by a XAML control, with data coming from a collection provided by some app state. This will cause a very memory-consuming object like a UIElement to stay around in memory until the app closes. The recommended pattern to avoid this is by using WeakReference preferable wrapped in an event manager / event proxy class. However this can be quite a lot of code to write, and the implementation is not straight forward, and will still cause the small proxy hang around at least until the event fires again (if it ever does).

It would be much simpler if a syntax alternative to "+=" would denote "weak". For example:

    staticInstance.Data.CollectionChanged += MyChangedHandler; //not weak event reference
    staticInstance.Data.CollectionChanged +~ MyChangedHandler; //weak event reference 

I'm not saying '+~' should actually be the syntax - just that we have something as simple as this to solve a very common scenario.

Uservoice suggestion for voting: http://visualstudio.uservoice.com/forums/121579-visual-studio/suggestions/7019993-provide-simple-syntax-to-create-a-weak-referenced

[svick]

What exactly would +~ do? Create that proxy class that doesn't completely avoid the memory leak? Or do you want some kind of "weak delegate" supported by the runtime? Or something else?

[sharwell]

@svick You can completely avoid the memory leak by using a ConditionalWeakTable to bind the lifetime of the delegate to the lifetime of the listener instead of the lifetime of the event source. Obviously this isn't a complete implementation explanation, but it does show that support for this concept exists within the current runtime.

[dotMorten]

@sharwell Thank you for the ConditionalWeakTable option. However it doesn't change the fact that this is a common problem that there should be a simple syntax for accomplishing.
@svick I'm fine with using a proxy, if using the method @sharwell proposes. That would mean it could be backwards compatible for framework, and be a compiler-only thing. Either way the goal of this proposal is to make it simple to create weak event handlers, so people can easier write good code that doesn't accidentally hang on to objects for too long.

[sharwell]

@dotMorten We are on the same page regarding the challenges of this functionality, which is essential for many kinds of applications.

A nuance of the +~ you describe is the requirement that it only be used in an instance member of a reference type, and it can only bind the weak reference to this. I imagine this could be very confusing for some users, and limiting for others. For example, suppose your application had a text window DiagnosticLog which could be opened by a user, and contained a DiagnosticLog.WriteLine method. A user might want to do this:

static void SomeUtilityMethod(DiagnosticLog log)
{
  AppDomain.AssemblyLoad +~ (sender, e) => log.WriteLine("Assembly Loaded");
}

How would you support attaching the weak event to log instead of this?

Other Approaches

The closest syntax you can have right now is the following:

environmentData.CollectionChanged += AsWeak<TEventArgs>(this, HandleCollectionChanged);

Where AsWeak is a method with the following signature:

static EventHandler<TEventArgs> AsWeak<TEventArgs>(object owner, EventHandler<TEventArgs> handler);

It would be safer to have a generic owner in order to use a generic type constraint and avoid boxing:

static EventHandler<TEventArgs> AsWeak<T, TEventArgs>(T owner, EventHandler<TEventArgs> handler)
    where T : class

However, this would increase the verbosity of usage:

environmentData.CollectionChanged += AsWeak<ThisType, TEventArgs>(this, HandleCollectionChanged);

Aside from your syntax, the use of weak event handlers would be simplified by improving the ability of the compiler to resolve the generic arguments for a call of this form.

[dotMorten]

@sharwell Wrt to your example

static void SomeUtilityMethod(DiagnosticLog log)
{
  AppDomain.AssemblyLoad +~ (sender, e) => log.WriteLine("Assembly Loaded");
}

You are declaring an anonymous delegate here, which goes out of scope immediately. This means it's up for garbage collection right away. In this scenario you would never want to do this anonymously. I can see how this might be confusing, but IMHO is very much expected and wanted behavior. Instead this would make more sense:

static void SomeUtilityMethod(DiagnosticLog log)
{
  EventHandler handler = (sender, e) => log.WriteLine("Assembly Loaded"); //pin handler to local scope
  AppDomain.AssemblyLoad +~ handler;
  //perform some assembly loading
  //

   //No need to remember to do this because of weak event:  AppDomain.AssemblyLoad -= handler;    
} //'handler' out of scope and unhooks from event.

[matwilko]

@dotMorten That may not hold up based on what optimizations the compiler might quite correctly perform.

If you don't use the handler local again after those initial two lines, the compiler has every reason to consider it "dead" because you never access it again, and so a valid optimization might be to simply reuse that local (in CIL terms) again if you define another EventHandler local further down the method.

Because of this, your assertion that you're "pinning" the handler in the local isn't true under all conditions, and it can still become immediately eligible for garbage collection.

[sharwell]

@dotMorten In the example I gave, the life of the EventHandler would be bound to the life of the receiver log, so it wouldn't prevent log from getting garbage collected but it would stay alive as long as log is otherwise alive.

[JohnnyBravo75]

This is one of my top wishes, because memory leak through event handlers are very common.

And the other way around? I declare an event as weak, not the subscribers, so the subscribers can only listen as long as the event is alive and fired?

public weak event EventHandler<CustomEventArgs> RaiseCustomEvent;

[svick]

@JohnnyBravo75 How is that different from normal events? An event is basically just a list of delegates. If the object that holds the list gets GCed, so will the delegates (and potentially also the subscribers).

[dotMorten]

@svick it's not different but the reference back often surprises people and causes stuff to "leak". And example is listening to events on static instances. Your class listening will stay in memory for the entire duration of the app. Quite often this isn't intended. Normally when you hold a reference to a static object its fine - your object can still be GC'ed. But because the event handler "points the other direction" this means the longer lived object unintentionally holds on to the shorter lived.
Another case is a UI control listening for property changes to a model object. The UI element is often short lived (especially for page navigating apps) but because the model data often stays alive in the lifetime of the app, if could be holding on to a lot of very memory expensive UI elements. There's a way to write the code for this but it's ugly, lot of code and thus also error prone. And more importantly developers never bother. Let's make it easy so they will.

[svick]

@dotMorten Yes, I understand that and I think that some way to fix that should be in the language.

My question was specifically aimed at @JohnnyBravo75's comment about his version of weak event where "the subscribers can only listen as long as the event is alive". Because that's how normal events already work, subscribers don't keep the object with event alive.

[whoisj]

An ideal way to manage this kind of thing would be to make WeakReference cheaper and easier to utilize. Effectively a way to keep a handle on an object until that object is no longer valid without the curse of keeping the object valid long beyond its intended use simply because you have a handle on it.

This is a bit of a crusade of mine because there's is no explicit way to free memory in C# and nearly all memory leaks are caused by people not realizing that they're keeping memory locked up long after they no longer need it (event handlers being a huge source of problems currently).

A weak reference should not impact GC logic, but should be set to null when the GC cleans up the memory associated with the allocation.

private weak object m_weak;

void Method(object solidObj)
{
    m_weak = solidObj; // the reference won't prevent garbage collection
}

If the community feels that call site markup is necessary to help explain / make more explicit / etc the assignment of weak references, then the ~ assignment, or similar, makes sense. Perhaps a specialized cast to weak (example below) makes more sense?

private weak object m_weak;

void Method(object solidObj)
{
    m_weak = weak(solidObj); // the reference won't prevent garbage collection
}

[svick]

@whoisj

An ideal way to manage this kind of thing would be to make WeakReference cheaper and easier to utilize. Effectively a way to keep a handle on an object until that object is no longer valid without the curse of keeping the object valid long beyond its intended use simply because you have a handle on it.

I'm confused, in what way doesn't WeakReference<T> already satisfy that?

A weak reference should not impact GC logic, but should be set to null when the GC cleans up the memory associated with the allocation.

Again, isn't that exactly what WeakReference<T> does?

[whoisj]

@svick yes it is what WeakReference<T>. However, WeakReference<T> is expensive, and in my opinion verbose and unwieldy. Having a language feature supporting weak references encourages their use, and use will encourage optimization of their performance.

Today, we have a cycle of "it's ugly, scary, complicated, and/or non-performant so I don't use them", followed by "nobody uses them, so it's low priority" followed by the initial statement.

[JohnnyBravo75]

@svick
"Because that's how normal events already work, subscribers don't keep the object with event alive"
Yes, I was not stating correct.
I meant, that instead of deciding on the subscribers side, that an reference is weak, I decide on the event side, that all subscriptions are automatically weak.

Suggested:
staticInstance.Data.CollectionChanged +~ MyChangedHandler; //weak event reference staticInstance.Data.CollectionChanged += MyChangedHandler; //not weak event reference

Alternative suggestion:
public weak event EventHandler<CollectionChangedEventArgs> CollectionChanged; staticInstance.Data.CollectionChanged += MyChangedHandler; // weak event reference (because event can only hold weak references)

When an event is declared as weak, all reference are automatic converted to weak references, the event holds only a list of weak delegate references.

Its just the matter, does the subscriber decide, or does the event decide that a reference is weak and this I wanted to suggest to think about.

[JohnnyBravo75]

@whoisj

I like your approach, because it is much more general and not limited to events, as I understand?

Btw: Big frameworks/programs often (also xaml/the bindingengine) make heavy use of weak references, without them they would leak.

[whoisj]

@JohnnyBravo75 given that events are subscription base, I suggest that they always use weak references. In fact, at the moment, I'm having trouble coming up with a reason they should not.

[dotMorten]

Events should definitely not default to weak. As pointed out higher up that would break a lot of existing code - the more obvious one anonymous delegates:

  myObject.MyEvent += (s,e) => { /* perform some code */ };

Ii think cases like this should result in a compiler error. Using weak events on handlers declared in local scope should not be allowed. Only event handlers that are member types of instances should be allowed (I don't even think static methods as handlers should be allowed, since that method would never be released, but I guess there's no harm in allowing it either).

@JohnnyBravo75 I don't see the point of declaring events as weak - that would mean an API would impose weak listeners on its users. And it wouldn't allow me to use weak listeners on existing events, like PropertyChanged and CollectionChanged events which are the most common culprits in XAML for this problem.

@whoisj Making all events weak by default would be a HUGE breaking change, and not an option.

[whoisj]

@dotMorten ahh and there's the example that I wasn't coming up with. Thanks for that.

Anonymous delegates aside, having event references be weak by default brings a lot of benefits. Perhaps the compiler could smart and helpful enough to track anonymous delegates as strong references - though that seems like a lot of book keeping for the GC to be doing.

Still, I stand by my assertion that weak references need to be first class citizens, used more often, and have a language feature to encourage their usage.

[miz-online]

If you don't want to wait for new syntax, here's a helper class that allows you to create weak delegates from my own class library (which I did not put on GitHub so far put plan to do 'if I have time' ;-)
https://gist.github.com/anonymous/bd9e28ee6c1fcdfd6bf0

[dotMorten]

@whiletrue-eu The implementation you have is pretty standard to solve this, but also quite syntax heavy to use. The outcome is people don't do it, when they need to - especially when dealing with XAML where you might be hanging on to big UI Pages due to this. Hence this suggestion.

Also the implementation has the downside of actually still leaking - it just leaks a much smaller object. If the event fires after the reference has been released, it will release this small bit of memory, but if the event never fires it'll never get cleaned up completely.
Having a proper weak event type that the GC has knowledge about could avoid this problem entirely.

[miz-online]

Hi Morten,
yes, you're right that it doesn't solve the problem completly and that it may happen that memory is leaked when the event doesn't fire anymore. But I think that it is better than the proposed weak reference manager solution that requires to save the weak delegate within. Your objects to keep them alive, because it is quite easy to introduce a bug if it is not clear that the event handling is 'weak' and the delegate is immediately collected.
I think the major issue is that not the event handler delegate must be weak, but the live span of the delegate must be bound to the target object. Not sure how this can be solved, but I am quite curious how this will be done. That's why I also voted for it.

Morten Nielsen [email protected] schrieb:

@whiletrue-eu The implementation you have is pretty standard to solve this, but also quite syntax heavy to use. The outcome is people don't do it, when they need to - especially when dealing with XAML where you might be hanging on to big UI Pages due to this. Hence this suggestion.

Also the implementation has the downside of actually still leaking - it just leaks a much smaller object. If the event fires after the reference has been released, it will release this small bit of memory, but if the event never fires it'll never get cleaned up completely.
Having a proper weak event type that the GC has knowledge about could avoid this problem entirely.

—
Reply to this email directly or view it on GitHub.

[demigor]

What's about WeakDelegate class, that uses WeakReference instead? And just does nothing, if target is not alive anymore.

Currently:
myObject.MyEvent += SomeEventHandler
converts to
myObject.MyEvent += new Delegate(SomeEventHandler)

Imagine that:
myObject.MyEvent += weak SomeEventHandler
just syntax sugar for:
myObject.MyEvent += new WeakDelegate(SomeEventHandler)

No CLR changes, small compiler changes, just a new Delegate-derived class and small syntax sugar.

[AlexPalma72]

I created something to resolve this some time ago, the code can been seen at https://github.com/MundusSoft/Mundus.Toolkit/blob/master/Mundus.Toolkit/WeakEventManager.cs.
it is use as extension methods i.e.
var subscription = this.WeakSubscribe(h => viewModel.PropertyChanged += h,
h => viewModel.PropertyChanged -= h,
(s, e) => s.HandlePropertyChanged(e.PropertyName));
then we need to keep a reference to that subscription or else the subscription will get dispose stopting the listening to the event.
I usually have my class (this on the example) implements the IDisposable an have a property List that gets iterated on the dispose method and call the Dispose for eaxh of the object on that list.
Also is important on the action that gets executed when the event fires not to use anything out of the parameters that get's passed in. "s" parameter is going to be the object that perform the WeakSubscribe in the example is the this if I was to do HandlePropertyChanged(e.PropertyName) instead of s.HandlePropertyChanged(e.PropertyName) then I would be creating a strong reference to the this class, defeating the purpose of using the WeakSubscribe, this is very important cause is easy to miss.

[dotMorten]

@AlexPalma72 Thanks. As mentioned above it is already possible and other people have similar solutions. That's not really what this is about. The solutions are all somewhat code-heavy. What this issue is trying to achieve is a simple syntax that automatically makes events weak, and makes it so easy people just do it and become used to it.

[Opiumtm]

I think weak keyword is a better option than +~ operator. weak keyword provide far better code readability.

someObject.Event += weak (sender, e) => {};
// translate to something like 
System.Runtime.WeakEvent.Attach(someObject, nameof(TSomeObject.Event), (sender, e) => {});

Same weak keyword may be used to simplify work with regular weak references

var obj = weak new List<string>();
// translate to 
var obj = new WeakReference<List<string>>(new List<string>());

public class AClass
{
    private weak List<string> weakList;
}

// translate to
public class AClass
{
    [WeakReference]
    private WeakReference<List<String>> weakList;
}

As WeakRefecence<T> can not contain null - assignment of null should be used to indicate weak reference itself is null.

List<string> hardList;
weak ListString<string> weakList = hardList;
// translate to
if (hardList == null)
{
    weakList = null;
} else
{
    weakList = new WeakReference<List<string>>(hardList);
}

Usage of declared weak references in code

// declare weak reference
weak List<string> weakList = new List<string>();
// same as
var weakList = weak new ListString();
// first declaration example is explicit variable type declaration, second - using "var" implicit typing

var cnt = weakList.Count;
// translate to:
int cnt;
{
    List<string> __weak_t;
    if (weakList != null && weakList.TryGetTarget(out __weak_t))
    {
        cnt = __weak_t.Count;
    } else
    {
        throw new InvalidWeakReferenceException();
    }
}

List<string> hardList = weakList;
// translate to
List<string> hardList;
if (weakList == null || !weakList.TryGetTarget(out hardList))
{
    throw new InvalidWeakReferenceException();
}

// use ?. operator to not throw exception if weak reference is invalid
var cnt = weakList?.Count;
// translate to
int? cnt;
{
    List<string> __weak_t;
    if (weakList != null && weakList.TryGetTarget(out __weak_t))
    {
        cnt = __weak_t.Count;
    } else
    {
        cnt = null;
    }
}

// adding ? at the end to not throw exception
List<string> hardList = weakList?;
// translate to
List<string> hardList;
if (weakList == null || !weakList.TryGetTarget(out hardList))
{
    hardList = null
}

// Checking if weak reference is alive by using ? operator
if (weakList? != null)
{
    Console.WriteLine("Weak reference is alive!");
}
// translate to
{
    List<string> __weak_t;
    if (weakList == null || !weakList.TryGetTarget(out __weak_t))
    {
        __weak_t = null;
    }
    if (__weak_t != null)
    {
        Console.WriteLine("Weak reference is alive!");
    }    
}

// Call a method
weakList.Clear();
// translate to
{
    List<string> __weak_t;
    if (weakList == null || !weakList.TryGetTarget(out __weak_t))
    {
        throw new InvalidWeakReferenceException();
    }
    __weak_t.Clear();
}

// Call a method not throwing an exception if reference is invalid
weakList?.Clear();
// translate to
{
    List<string> __weak_t;
    if (weakList != null && weakList.TryGetTarget(out __weak_t))
    {
        __weak_t.Clear();
    }
}

And types

var t = typeof(weak List<string>);
// same as
var t = typeof(WeakReference<List<string>>);

weakList.GetType() // result: typeof(WeakReference<List<string>>)
weakList?.GetType() // result: typeof(List<string>)

[gafter]

The LDM discussed this. Most of the solution needs to be in a library. And given such a solution, language support adds little value. We do not intend to move forward with this as a championed proposal for a C# language change.

[dotMorten]

@gafter Could you please elaborate on what was discussed? I don't understand why this needs to be in a library. We can already write it in code, and its tedious ugly code - even after using helper-methods in a library, and that still creates tiny memory leaks.

The entire purpose of the language proposal was to help people consider avoiding these types of memory leaks by making a much much easier no-brainer way to do this.

[gafter]

@dotMorten There is little that the compiler can do that would avoid those tiny memory leaks.

@mattwar Can you expand on this, perhaps pointing to the blog posts?

[whoisj]

@dotMorten There is little that the compiler can do that would avoid those tiny memory leaks.

The memory leaks are a implementation problem, but I do not believe that's what @dotMorten is concerned with. As I understand it, he's looking for syntactic sugar that makes use of weak references cleaner to write/read.