Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Provide built-in support for "weak event" pattern #18645

Open
Opiumtm opened this issue Sep 20, 2016 · 57 comments
Open

Provide built-in support for "weak event" pattern #18645

Opiumtm opened this issue Sep 20, 2016 · 57 comments
Labels
api-needs-work API needs work before it is approved, it is NOT ready for implementation area-System.Runtime
Milestone

Comments

@Opiumtm
Copy link

Opiumtm commented Sep 20, 2016

"Weak event" pattern is a widely used pattern. But still, there is no out-of-the-box support for it in .NET

@svick
Copy link
Contributor

svick commented Sep 20, 2016

How would it look like? How would it be implemented?

@Opiumtm
Copy link
Author

Opiumtm commented Sep 20, 2016

@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.

@svick
Copy link
Contributor

svick commented Sep 20, 2016

Related proposal to make weak event handlers a language feature: dotnet/roslyn#101.

@svick
Copy link
Contributor

svick commented Sep 20, 2016

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.

@Opiumtm
Copy link
Author

Opiumtm commented Sep 20, 2016

@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.

@shmuelie
Copy link
Contributor

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

@Opiumtm
Copy link
Author

Opiumtm commented Sep 21, 2016

@SamuelEnglard sadly, it wouldn't be usable on .NET Native builds, because .NET Native doesn't support IL emit at runtime.

@shmuelie
Copy link
Contributor

@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

@svick
Copy link
Contributor

svick commented Sep 21, 2016

@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.

@thomaslevesque
Copy link
Member

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.

@shmuelie
Copy link
Contributor

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

@thomaslevesque
Copy link
Member

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.

@shmuelie
Copy link
Contributor

shmuelie commented Sep 22, 2016

@thomaslevesque Good point, give me a minute and I'll upload my test

TestRunner.cs is what I've been using to test with

@thomaslevesque
Copy link
Member

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 👍

@shmuelie
Copy link
Contributor

Exactly. Pretty much just replace usage of the static members of Delegate with my WeakDelegate was the goal

@Opiumtm
Copy link
Author

Opiumtm commented Sep 22, 2016

@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.

@shmuelie
Copy link
Contributor

I should be able to have a speclet for this soon based on an updated version of my gist

@shmuelie
Copy link
Contributor

Wanted to give an update that I moved my work to https://github.com/SamuelEnglard/weakdelegates

@shmuelie
Copy link
Contributor

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

@AlexGhiondea
Copy link
Contributor

I think this is an interesting idea and it might be worth pursuing.

@terrajobst what do you think?

@jnm2
Copy link
Contributor

jnm2 commented Dec 23, 2016

While we're throwing things in, here's what I've had. An implementation using WeakDelegate, and one using ConditionalWeakTable. Thread-safe.

@thomaslevesque
Copy link
Member

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;

@dotMorten
Copy link

@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
Copy link
Member

@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.

@JaggerJo
Copy link

JaggerJo commented Jan 29, 2019

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

@thomaslevesque
Copy link
Member

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

@JaggerJo
Copy link

Thank you, this might help a lot.

I’m working mostly in F# so I have to test if your implementation “feels right”.

@davidmilligan
Copy link

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.

@dotMorten
Copy link

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

@davidmilligan
Copy link

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.

@msftgits msftgits transferred this issue from dotnet/corefx Jan 31, 2020
@msftgits msftgits added this to the 5.0 milestone Jan 31, 2020
@maryamariyan maryamariyan added the untriaged New issue has not been triaged by the area owner label Feb 23, 2020
@koszeggy
Copy link

koszeggy commented Apr 7, 2020

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?

@davidmilligan
Copy link

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.

@thomaslevesque
Copy link
Member

@koszeggy that only works if the event publisher has a shorter lifespan than the subscriber. It's not always the case.

@koszeggy
Copy link

koszeggy commented Apr 7, 2020

@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.

@dotMorten
Copy link

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.

@koszeggy
Copy link

koszeggy commented Apr 7, 2020

@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)

@dotMorten
Copy link

@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)

@koszeggy
Copy link

koszeggy commented Apr 8, 2020

@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:
  1. Managed objects that implement IDisposable. [...]
  2. 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.

@dotMorten
Copy link

dotMorten commented Apr 8, 2020

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.

@TheXenocide
Copy link

@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.

@dotMorten
Copy link

dotMorten commented Apr 16, 2020

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.

@joperezr joperezr modified the milestones: 5.0.0, Future Jul 6, 2020
@joperezr joperezr removed the untriaged New issue has not been triaged by the area owner label Jul 6, 2020
@jbe2277
Copy link
Contributor

jbe2277 commented Dec 30, 2020

Yet another WeakEvent implementation provided by: System.Waf.Core.
Motivation

  • Independent of a specific application model. Must work with: WPF, Xamarin (Forms), MAUI, UWP, WinUI.
  • Support AOT compilation (e.g. Xamarin.iOS, UWP .NET Native).
  • Usable with existing publishers (sources) without modifying them -> Event handler subscription declares it to be weak.

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.

@robloo
Copy link

robloo commented Jan 15, 2022

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.

@thomaslevesque
Copy link
Member

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.

@dotMorten
Copy link

Yes default as weak would cause more problems than we're trying to solve.

@robloo
Copy link

robloo commented Jan 15, 2022

Yes, it would break things. Honestly though, weak events should have been the default from the beginning and strong event references used opt-in (of course still supported). Every single large app ends up having to roll their own solution for weak events and it quickly becomes the default event pattern.

@dotMorten
Copy link

dotMorten commented Jan 15, 2022

While I whole heartedly disagree, there’s really no point in even discussing it as such a breaking behavior is almost sure to be a non starter. I could come up with numerous examples where such a behavior is going to create very unpredictable results

@robloo
Copy link

robloo commented Jan 15, 2022

I'll drop it as I agree with your assessment its a non-starter. I would say if things were designed differently from the start app architecture changes can avoid any such unpredictability while also avoiding common memory leaks which defeat the whole point of a managed runtime. (I also said the strong pattern could be kept opt in where its easier for things) This needed to be rethought at a fundamental level so I would encourage letting go of pre-conceived patterns. Anyway, thats all for a future language/runtime after us.

@legistek
Copy link

I'm glad I found this issue and that it's still open. Count me in as a vote for this.

I agree with the comment that weak events should have been the default from the beginning. (I'm also not sure how changing this now would be breaking - is there really code out there in the wild that relies on objects being kept alive solely by virtue of an event subscription? Talk about asking for trouble. But it's a moot point of course as it'll never happen.)

Certainly we all can make our own Weak Event patterns. WPF does it and that pattern can be copied easily. There are plenty of others. How efficient they are is dubious. What they indisputably are is ugly and hackish.

Being able to simply say:

obj.AnyEvent += my_handler weak;

would just be so delightful.

@thomaslevesque
Copy link
Member

I agree with the comment that weak events should have been the default from the beginning. (I'm also not sure how changing this now would be breaking - is there really code out there in the wild that relies on objects being kept alive solely by virtue of an event subscription? Talk about asking for trouble. But it's a moot point of course as it'll never happen.)

I'm pretty sure it would break a lot of existing code. You might not consciously "rely on objects being kept alive solely by virtue of an event subscription", but in practice, it happens a lot, as soon as you start using lambda expressions to subscribe to events. If you do something like this:

void Subscribe(int foo)
{
    something.SomeEvent += () => Console.WriteLine(foo);
}

Pretty familiar, right? Nothing suspicious here?
But the event handler captures a local variable, so the compiler actually creates a closure class with an instance field for foo, and the lambda becomes an instance method of that class. So it's translated to something like this:

void Subscribe(int foo)
{
    var closure = new Closure { foo = foo };
    something.SomeEvent += closure.Lambda;
}


class Closure // Actually named something like YourClass+<>c__DisplayClass8_0
{
    public int foo;
    public void Lambda() // Actually named something like <Subscribe>b__0
    {
        Console.WriteLine(foo);
    }
}

Once the Subscribe method returns, the closure instance is only referenced by the event publisher. So if it were a weak event, the instance would be garbage collected, and the event handled would no longer be executed. Oops!

@legistek
Copy link

@thomaslevesque ah that's interesting! I would've guessed the class instance (or static class as the case may be) on which Subscribe(int foo) was called would hold a reference to the Closure instance for at least its own lifespan - for exactly this reason - but if not then yes I definitely see your point.

That raises an interesting point then about a hypothetical weak keyword for event subscriptions. If I understand you right, it would basically be pointless to create a weak event subcription with lambdas because in the vast majority of cases no one would hold a strong reference to them and they'd get GC'd right away. It would only make sense - and maybe should only be allowed - for actual class methods then.

@igor84
Copy link

igor84 commented Sep 3, 2023

I don't see anyone mentioning this so I am wondering if I am missing something.

If we would get the ability to do weak subscribes and we start to rely on it, from time to time it might happen that although subscriber is not referenced by anything and is "disposed" and ready to be collected by the GC that just didn't happen yet and the publisher sends the event at that time. That would mean that subscribed method would still run on the subscriber and it might not expect it if it is in some disposed state. Is that right? Seems like that would lead to random bugs that are very hard to reproduce.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-needs-work API needs work before it is approved, it is NOT ready for implementation area-System.Runtime
Projects
None yet
Development

No branches or pull requests