dotnet / csharplang

The official repo for the design of the C# programming language
11.56k stars 1.03k forks source link

[Proposal]: One-time event subscription #4166

Open Pyrdacor opened 3 years ago

Pyrdacor commented 3 years ago

One-time event subscription

Summary

Basically a syntax addition where an added event handler removes itself automatically from the event on execution. So it subscribes only to the first event raising.

Motivation

Fire and forget events are a useful use case. Especially in games there are many use cases like starting an animation/sequence and tell me when it has finished. Adding an event handler to a finished event of an animation class instance could be an approach. But once finished I don't want the handler to be attached to that event any longer.

This is already possible by removing the hander from the event inside the handler code itself but a) it can be forgotten easily and b) this isn't possible to my knowledge with anonymous lambdas which are often used in those cases.

Detailed design

Basically I think of something like this:

public class Animation
{
    public event Action Finished;
}

Animation anim = new Animation();
anim.Finished += once () =>
{
    // do something
};

This code is equivalent to:

Animation anim = new Animation();
Action handler;
handler = () =>
{
    anim.Finished -= handler;
    // do something
};
anim.Finished += handler; 

And so the compiler could just generate such replacement code. But as a programmer I don't want to add the whole boilerplate code and I don't want to create temporary handler instances just to make this possible. I could also use local functions of course but even then I always have to add the handler remove line and if I forget it, it might lead to behavior I wouldn't expect. For example if the animation is reused by other code. Moreover why should I use named local functions if I only use them once?

Moreover a keyword like once strongly state what the intention is and it is used where I expect it to be. At the place of attachment/subscription. If I replace the handler I have to reinsure that the unsubscription is also added. This is not necessary with the new syntax because it is independent of the handler.

Things like Observables have similiar functionality or at least can accomplish them very easily. So I guess this would be an easy and useful extension.

Event chains will be possible very easily as well without the possibility to forget any unsubscribing along the way.

Drawbacks

I don't see any.

Alternatives

Wrapping local functions or actions. Avoiding anonymous lambdas.

But they all won't remove manual unsubscribing inside the handler which could be easily forgotten.

Unresolved questions

The syntax is not yet clear. Maybe someone else has a good idea.

JustNrik commented 3 years ago

If the event is invoked only once, why not just set it to null after it's invoked? if so, then the anonymous delegates will be freed for the GC to collect, this doesn't require any new language support. Unless I'm missing something.

Example

public class Test
{
    public event Action? Closed;

    // other members

    public void Close()
    {
        if (Closed is null)
            return;

        // other logic

        Closed.Invoke();
        Closed = null;
    }
}
Pyrdacor commented 3 years ago

The event can be reused and therefore be called multiple times. But a subscriber should only subscribe for the first execution after subscribing.

But you are right. I should rename this to one-time subscription.

Pyrdacor commented 3 years ago

Here is another example.

I want to play a special animation. When it is finished I will execute some logic and play another animation. When this last animation is done I execute some other logic.

Then another piece of code wants to play the same animation and wants to execute some code when it is finished as well.

So both code pieces reuse the same animation instance and its Finish event but each of them only once so they won't execute their code if another caller plays the animation as well. Of course this is in a non-parallel environment so the animation is only played once at the same time.

You can also think of some sequences that uses chained events and inside the chain the animation is played multiple times but different code is executed after each one.

This might be also possible with passing handlers around but why not just add the possibility to events? One-time subscriptions are not a rare use case imho.

moveAnimation.Finished += once () =>
{
    ShowMessage("Foo attacks Bar");
    attackAnimation.Finished += once () =>
    {
        ShowMessage("Foo did 5 damage to Bar");
        moveAnimation.Finished += once () =>
        {
             //... 
        };
        moveAnimation.Start();
    };
    attackAnimation.Start();
};
moveAnimation.Start();

An even better syntax for this or event chaining could be added as well but this would be an improvement nonetheless.

HaloFour commented 3 years ago

IMO this is a pretty niche thing to want to do and saves very little over the existing solution. If there was going to be any enhancements to events in this area I'd rather have something like this which seeks to make it easier to write method that work with events which would enable composing them in a variety of ways.

Pyrdacor commented 3 years ago

I would like to see some general improvements to events. Thinking of things like Rx etc the C# events need some love. But I am not sure if one-time subscriptions are a niche use case or if this is only the case because there was no such feature in the first place. When I think about it I often used similar constructs in the past. Events are not very cool to use but they are useful nonetheless.

CyrusNajmabadi commented 3 years ago

An event here seems like potentially the wrong metaphor. Why not just have a special registration method where you pass in your delegate and internally you ensure you only call it once and then release it

JustNrik commented 3 years ago

I would like to see some general improvements to events. Thinking of things like Rx etc the C# events need some love. But I am not sure if one-time subscriptions are a niche use case or if this is only the case because there was no such feature in the first place. When I think about it I often used similar constructs in the past. Events are not very cool to use but they are useful nonetheless.

You can easily create a helper class that clears the list of handlers when the event is invoked, so this feature is doable without any new syntax...

public class OneTimeSubscriptionEvent<THandler> where THandler : Delegate
{
    private readonly HashSet<THandler> _handlers = new();

    public void Add(THandler handler)
        => _handlers.Add(handler);

    public void Remove(THandler handler)
        => _handlers.Remove(handler);

    public void Invoke<TArgs>(Action<THandler, TArgs> action, TArgs args)
    {
        foreach (var handler in _handlers)
            action(handler, args);

        _handlers.Clear();
    }
}

usage:

private readonly OneTimeSubscriptionEvent<Action<CoolEventArgs>> _foo = new();
public event Action<CoolEventArgs> Foo
{
    add => _foo.Add(value);
    remove => _foo.Remove(value);
}

public void MethodThatInvokesYourEvent()
{
    // stuff

    _foo.Invoke((handler, args) => handler.Invoke(args), new CoolEventArgs(something));

    // more stuff I guess
}

you can adapt it to your needs

Pyrdacor commented 3 years ago

An event here seems like potentially the wrong metaphor. Why not just have a special registration method where you pass in your delegate and internally you ensure you only call it once and then release it

In my understanding when somethings ends this is an event. Of course I can work around it with registration, passing handlers etc. But using an event seems much more intuitive in such situation. And your approach would also require a bunch of stuff I have to implement on my own.

Pyrdacor commented 3 years ago

@JustNrik Sure but this adds additional code to each event and event raising. Just to allow a simple one-time subscription. I know I can workaround this. I even did. But not in a way that is very pleasing.

svick commented 3 years ago

@Pyrdacor

An even better syntax for this or event chaining could be added as well but this would be an improvement nonetheless.

That already exists, in the form of async-await. Using that (and some changes to the animation library), your sample could be written like this:

await moveAnimation.Run();
ShowMessage("Foo attacks Bar");
await attackAnimation.Run();
ShowMessage("Foo did 5 damage to Bar");
await moveAnimation.Run();
//...
Pyrdacor commented 3 years ago

In my case the animation progress is controlled by the main loop so it can't be an async Run method. Event based programming is not equal to async programming.

svick commented 3 years ago

@Pyrdacor I didn't say Run should be an async method, though it will be used in one. What you can do is to use TaskCompletionSource to create a bridge between the two worlds. Basically, Run() could be an extension method that looks like this:

Task Run(this Animation animation)
{
    var tcs = new TaskCompletionSource(); // or use the generic version if you're not on .Net 5 yet
    Action handler = null;
    handler = () =>
    {
        animation.Finished -= handler;
        tcs.SetResult();
    };
    animation.Finished += handler; 

    animation.Start();
    return tcs.Task;
}
Pyrdacor commented 3 years ago

This is a nice wrapper. But it will force me to make the method async which calls Run. Don't get me wrong. I know there are ways to implement this. My examples are at the end just examples. The core of it is just built-in support for a one-time subscription. But if you think it isn't worth considering I will just stick to such implementations.

rootflood commented 3 years ago

i had some librarys for wait until handle a delegate but with much performance issue other problem is i must write a wrapper for every parameters of delegates

    public class Test
    {
        public delegate ref t GetRef<t>();
        public static Task WaitForHandle(GetRef<Action> Action)
        {
            var Task = new Task(() => { });
            Action MyAction = null;
            MyAction = () => {
                Task.Start();
                Action() -= MyAction;
            };
            Action() += MyAction;
            return Task;
        }

        public static void WaitForHandle(GetRef<Action> Action, Action Handle)
        {
            var Task = new Task(() => { });
            Action MyAction = null;
            MyAction = () => {
                Handle();
                Action() -= MyAction;
            };
            Action() += MyAction;
        }

        public static Action Event;
        public static void main()
        {
            // wrap event
            WaitForHandle(() => ref Event);

            // Make Other event For Handle Once
            WaitForHandle(() => ref Event, () => Console.WriteLine("Handled"));
        }
    }

but this is better performance, usable any where and readable better

anim.Finished += once () =>
{
    // do something
};

the problem is lock the object, so if two threads run at same time Finished will run two time here i think this make code very very complicated without a syntax or library here and developer will need take care a lot of problems for every event which need wrap or ... this is very much hard in medium and big projects and will have a lot of logical bugs in run time

Pyrdacor commented 3 years ago

Yeah. Creating an event class with all the fancy stuff would be fine for me but as you mentioned (if I get you right) the big advantage of events is that they magically accept all delegates and the associated invoke call will have the right signature to pass the parameters. If there would be a possibility to accomplish this without events we could forget about events altogether and use some event class from some library. But adding delegate type dependent function signatures won't be a possibility for C# at all I guess.

ClosetBugSlayer commented 3 years ago

Automatic un-subscription would help get rid of the biggest culprit of memory leaks I have ever seen in production code: orphaned strong reference event handlers. Most objects (especially INotifyPropertyChanged viewmodels and WPF Dispatcher) don't set events to null on disposal, even when there is no possible opportunity for the event to ever again be triggered. Blind faith in the GC to that extent disturbs me.

canton7 commented 3 years ago

I second the observation that this is a poor man's way of writing an async method.

What you have now is equivalent to an async void method, but written using nested callbacks. It has the same characteristics as an async void method: the caller cannot listen for completion, and exceptions have no nice route back to the root of the application.

So I can't see any advantage of writing your code using nested callbacks, over using an async void method. A Task is conceptually just an event which can only happen once (and can fail / be cancelled as well as succeed, but you don't need to use this). Awaiting a Task basically involves registering a callback which is invoked when the Task completes. Tasks have strong support from both the language and the runtime: I don't think we need an additional concept which does the same thing, but less powerful.

It's worth noting that Xamarin.Form's animation system also uses await, so there's strong precedent here.

Pyrdacor commented 3 years ago

@canton7 Don't focus on the nested callback thing. It is about one-time subscriptions.

But if you want to know what might be an advantage. The events/callbacks can be triggered from a definite place like a main update loop. Of course this behavior could also be implemented with async in some way but events are much more intuitive.

quinmars commented 3 years ago

I'd go with ReactiveExtensions:

using System.Reactive;

public class Animation
{
    private readonly Subject<Unit> _finished = new Subject<Unit>();
    public IObservable<Unit> Finished => _finished;
}

Animation anim = new Animation();
anim.Finished.Take(1).Subscribe(_ =>
{
    // do something
};

To invoke the event you have to call _finished.OnNext(default) instead of Finished?.Invoke().

tonygiang commented 3 years ago

I would like to add an additional vote of approval for this syntactical sugar, whatever shape it may take. We have been rolling our own solution when it comes to a pattern of avoiding duplicated executions. In our domain, i.e multiplayer networking, we often encounter situations where a competitive match would have to forcefully end in case of any disconnection. We only want to log the result of the match only once on an OnClientLeave event, which could be triggered by any of the N clients in a room disconnecting, else we would have N records of the same match when all clients eventually leave the room.

Thaina commented 3 years ago

I too think that this is too niche and it was already cover with RX pattern

I would like to support the idea of introducing RX pattern into C# language and this once would be included too (but would be difference syntax)