One-shot events in C#

Question

I have a class that performs some long-running operation.

This class exposes a Completed event. I'd like clients that use this class to register to get notified ONLY ONCE when the class completes doing its operation.

Would you consider this code appropriate to handle this case:

public class TutorialController
{
    public event Action Complete;

    protected void OnComplete()
    {
        if (Complete != null)
        {
            Complete();
        }

        // Reset to null.
        Complete = null;
    }
}

The usage of "client" code is:

TutorialController controller = new TutorialController();
controller.Complete += ... // Register to get notified.

// Once this is done, it will raise Complete() and clear it.
controller.DoLongOperation();

Due to the current design, the Complete event actually triggers another callback that is passed from the outside:

public void SomeMethod(Action action)
{
    // Code similar to this.
    controller.Complete += () => { action(); };
}

The action passed in is ONLY RELEVANT for the current invocation of the controller, that is - after it would run and complete once, it's no longer needed to notify me of this event (with the current "action" parameter). The next invocation will pass its own action, and so on.

Note that the event sender is an object that is kept around throughout the lifetime of the app, otherwise it would be collected and each time I would get a new copy and register on its event.

Answer 1

Other answers are great, but this is really re-inventing the wheel. What you are looking for is the observer pattern. .NET has great in-built capabilities with observer through System.Observer<T> and System.Observable<T>. You can also get the library Rx-Main through NuGet to gain the ability to compose these, LINQ-style, into operations that can be filtered or exploded or modified as you see fit.

In .NET's Observable<T>, the Observable<T> produces values once it is subscribed to (unless it is a hot observable - that's out of scope of this answer). It will then send an OnComplete 'message' to the subscribed Observer<T> when it has finished. It also has the ability to send an OnError(Exception) notification to the Observer, but only if the exception occurs during the subscription. Exceptions during observation are not implicitly caught.

Before we get onto that, it's worth nothing that you also have Task<T>. It's really worth realizing what you are trying to do here. I feel that putting a Completed event on your class is a code smell as it's breaking encapsulation, and you should really bind the completion to the lifecycle of the method invocation (i.e, make it a return). Semantically speaking, if your method will only produce one result (i.e, "I've finished"), then you should return a Task (or Task<T>). This lets users use async/await.

If your method is going to have a sequence of results before finishing (i.e, it's got an IEnumerable perhaps?) then you should return an IObservable<T>.

Here's how I would lay it out in both cases.

public class TutorialController
{
    public async Task DoLongOperation()
    {
        return Task.Run(() => ....);
    }
}

Task.Factory.StartNew(controller.DoLongOperation()).ContinueWith(() => ...);

Replace the ellipsis with the rest of your DoLongOperation method's code. If your DoLongOperation needs to return a single result, change the return to a Task<T>. By the way, you should only use Task.Run if your long operation is CPU-bound or partially IO/CPU bound.

If your DoLongOperation returns multiple results - for example, it's getting a list of things from an external resource such as a web resource - you should instead return IObservable<T>. With ReactiveExtensions, the asynchronous composition is very useful and makes IObservable<T> better than Task<IEnumerable<T>> in this case.

public class TutorialController
{
    public IObservable<T> DoLongOperation()
    {

        return Observable.Create(() => ...);
    }
}

var results = Observable.Subscribe(p => OnNext(p), e => OnException(e), () => OnCompleted());

IObservable<T> leaves the a/synchronous nature of the call up to the caller, so you don't need async on this method.

If you provide the rest of your code for your DoLongOperation I can help convert it to IObservable. But really, the code you have right now seems like a hugeeee code smell. For one, it's mutable, and it's DEFINITELY not thread safe.

By the way one guarantee this code gives you over say @Memleak's answer is that you can invoke Subscribe on a completed Observable and no exceptions will be thrown there is no opportunity for side-effects, which is key in asynchrony. This is infinitely better than having two separate units of functionality in a single function based on whether something has already been Disposed/Completed or not. This adheres to the idea that disposables should fail silently if they have already been disposed of.

As @Jeroen rightly mentioned in chat, .NET events are an implementation of the observer pattern. But Rx's IObservable / TPL's Task allows you to compose asynchronous events and has a notion of completeness - .NET events do not. This is more useful for your use case.

Answer 2

For me this whole idea smells like a bad design. I don't like having this logic hidden in the event sender. If I see an event and I subscribe to it I really expect to be called each time that actions happens. Would be really weird, at some point, to simply have my delegate removed from the list.

And this is because events shouldn't really have so much control on the callers. If I really need a one-shot event I would most definitely let the caller take that decision and unsubscribe itself.

Semantically speaking, an event is a class yelling "DUDES, I JUST DID whatever" and no longer cares about what happens with that information. Whoever was interested heard it and can act accordingly. So if you just completed the Tutorial then simply let everyone know you completed it.

Now, let's get on the more interesting topic and that is why do you need a one-shot event? I see the following cases:

1. The receiver is interested only in the first occurrence of an event. Then let the receiver decide when and under what circumstances it will remove itself from the event. Maybe at some point you will have a receiver interested in the first two occurrences. Or based on some weird circumstances decides to not handle the current event and waits for another one.

2. The sender switches contexts during two events therefore the second call is no longer valid for the initial listeners. Then you have an object that does too much. Refactor such as the lifetime of the sender matches the context from which he triggers the event.

3. The sender will trigger the event multiple times but only the first one is valid. This is a bug and must be fixed properly not patched.

I don't know your specific use-case so I can't write some good suggestions, but in any case, I would definitely let the users of my events decide when and under what circumstances they unsubscribe.

Now, if you would be so kind to provide us with a proper use case of why is this design needed I would be more than happy to extend my review for that as well.

---

Edited based on new question details:

I would modify TutorialController like so:

public class TutorialController
{
    public bool IsCompleted { get; private set; }

    //Make this public, private or protected
    //  depending on the use case.
    // This will most likely be called from your other event.
    public void SetCompleted()
    {
        if (IsCompleted)
        {
            //handle this case...
        }

        IsCompleted = true;
        OnComplete();
    }

    // Action triggered at the moment of completion.
    public event Action Complete;

    protected void OnComplete()
    {
        if (Complete != null)
        {
            Complete();
        }
    }
}

OnComplete will be triggered only once, at the moment of completion. Users of this class can check if the tutorial is done by calling IsCompleted otherwise they can wait for the completion by listening to OnComplete. Clean and simple.

Answer 3

Unrelated to your main question, but for thread safety, it's recommended you take a local copy of the event, like so:

var handler = Complete;
if(handler != null)
{
    handler();
}

A quick reference as to why (from the linked question above):

Events are really syntactic sugar over a list of delegates. When you invoke the event, this is really iterating over that list and invoking each delegate with the parameters you have passed.

The problem with threads is that they could be adding or removing items from this collection by subscribing/unsubscribing. If they do this while you are iterating the collection this will cause problems (I think an exception is thrown)

The intent is to copy the list before iterating it, so you are protected against changes to the list.

Assigning the event handler to a variable like this works because events are immutable and are essentially Multicast Delegates and subscribing / unsubscribing it actually makes a copy of the invocation list at that point in time.

Although bear in mind any registration or deregistration of events after the copy of the handler is made will be ignored during the execution of the copied handler.

Answer 4

Would you consider this code appropriate to handle this case?

There is the Principle of least astonishment to consider. Or, "If is looks like a duck and quacks like a duck, it's usually a duck".

To the user of your interface your event looks like a typical event. Typical events don't automatically unregister event handlers. Not only does the interface not imply that the event handlers are unregistered after the event is fired, but the interface actually implies that the handlers are not unregistered.

Answer 5

Your code will work, but you are using events in a way they should not be used. Once they are public, they are for signing and unsigning as per user of the class (those that start the operation), not to be unsigned by the class (controller). If you still for whatever reason need to preserve the class (and its state) and call the Complete action once, it should be a parameter of the start method:

public class TutorialController
{
//  note I have changed it to private, that nobody can see it from outside
    private event Action Complete;
//  flag that will help us to prevent calling start when previous not finished
    private bool started;

//  and here is how we subscribe and start the operation
    public void DoSomeAction(Action complete)
    {
    //  this will prevent starting the action when previous was not finished
        if(Interlocked.CompareExchange(ref started, true, false) == true)
            throw new Exception("Still working");
    //  sign for the event if you want to call other method without passing it
        Complete += complete
    //  start the thread actually doing the work (just an example)
        new Thread().Start(this.DoTheWork);
    }
//  asynchronous work here
    private void DoTheWork() {
    //  some heavy work here

    //  unsubscribe, flag and call at the end
        Action action = Complete;
        Complete = null;
        started = false;
        if(action != null) action();
    }
}