10
\$\begingroup\$

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.

\$\endgroup\$
  • 2
    \$\begingroup\$ dont raise the event again if task is already completed (check it some where). why making suscriber null, \$\endgroup\$ – paritosh Sep 8 '14 at 9:57
  • \$\begingroup\$ If I understood correctly you want to trigger the event only once, is that correct? Any specific reason for not doing exactly that? Are there any scenarios in which someone might subscribe after the scenario is completed but still needing that event? \$\endgroup\$ – Memleak Sep 8 '14 at 10:59
  • \$\begingroup\$ @Memleak I want to register to the event using some callback, invoke the event, and that's it. Next time, i will register again, with another callback, and wait for the event, and so on. \$\endgroup\$ – lysergic-acid Sep 8 '14 at 12:40
  • 1
    \$\begingroup\$ In addition to my answer below I'd like to point out that DoLongOperation sounds like exactly the kind of method that could use Task. This is exactly what asynchronous programming was created for. \$\endgroup\$ – Dan Pantry Sep 8 '14 at 13:38
  • \$\begingroup\$ @DanPantry You're right! Problem is, the environment we're working on (Unity3d engine) does not have that. But we could create our own Task-like infrastructure. \$\endgroup\$ – lysergic-acid Sep 9 '14 at 6:36
20
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ I like this solution as it is much cleaner and makes the "two state object" TutorialController explicit. Indeed there is a code smell in the method name, in the fact that another event is triggering this event and potentially can do it after it was completed and so on. Actually getting rid of that two states and refactoring with SRP in mind simply removes the problem at all, since whoever is managing these objects knows for sure when the transitions occurs and what needs to be done. But the design change for this might be too much. \$\endgroup\$ – Memleak Sep 8 '14 at 13:38
  • \$\begingroup\$ I think OP should at the least refactor to Task given the name of his method and what that implies. If it is a long-operating method it should definitely be a Task using async/await, rather than using an Event. Using events for asynchrony (this should be a word) is mostly deprecated with the advent of TPL, even in the .NET API. As for the design change being too much, maybe, but then why ask for code review if you aren't willing to break a few things ;-) \$\endgroup\$ – Dan Pantry Sep 8 '14 at 13:40
  • \$\begingroup\$ Even from an "event" perspective, it would seem much cleaner to attach an event to a one-time use object which fires its events and then "dies", than to clear the events from an object which stays alive. If an object exposes an event which is supposed fire when it dies, I would consider it semantically reasonable to have the object say that its cleanup consists of firing events and detaching events. A subscribe request received at any time after cleanup has commenced should execute and detach the supplied callback at first opportunity, just like any callbacks that have been... \$\endgroup\$ – supercat Sep 8 '14 at 19:57
  • \$\begingroup\$ ...attached earlier but haven't yet run. Such a design would allow for thread-safe usage patterns even if an object might asynchronously die before the client asks it to attach its event. \$\endgroup\$ – supercat Sep 8 '14 at 20:00
13
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Thanks for your answer. I have updated my question, hopefully this sheds some light on what i'm trying to do. \$\endgroup\$ – lysergic-acid Sep 8 '14 at 10:57
  • \$\begingroup\$ This answer uses a lot of code (thus brittle), and not very thread-safe if SetCompleted is called from more than one thread.. \$\endgroup\$ – Dan Pantry Sep 8 '14 at 13:44
  • \$\begingroup\$ I wrote this as a concept without making any assumptions about the rest of the code base or existing design and working with what is given in the question. Even in this form, my feeling is that TutorialController breaks the single responsibility principle and feels like there is some kind of weird connection between whoever talks with this controller and the controller itself. Even the fact that a Controller has a Completed concept seems fishy. But for the given design and only the given code I would definitely make the Completion status explicit as in my answer. \$\endgroup\$ – Memleak Sep 8 '14 at 13:56
  • \$\begingroup\$ I agree with you there, this class definitely violates SRP, and I don't feel like the existing solution OP has is very conducive, so I altered it significantly. Was just concerned about thread safety due to the nature of the method name, that's all \$\endgroup\$ – Dan Pantry Sep 8 '14 at 13:59
7
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Assigning the delegate will not copy it. I suppose you wanted to demonstrate that it should be copied ? \$\endgroup\$ – lysergic-acid Sep 8 '14 at 10:51
  • 1
    \$\begingroup\$ According to the answer to this question: stackoverflow.com/questions/3668953/… "MulticastDelagates are immutable, so if you first assign a variable, null check against the variable and invoke through it, you are safe from that scenario" \$\endgroup\$ – Nick Udell Sep 8 '14 at 11:06
  • 1
    \$\begingroup\$ That's not an immutability guarantee. Assigning the event handler to a variable does not copy it unless the event handler is a value type. Otherwise you will just be copying the reference, which is not guaranteeing anything. if (Event != null) Event(...) is the correct usage. Assigning to a local variable will make no discernible difference. The correct way to prevent this is to encapsulate the event handler correctly, and prevent external members from mutating the member in an unsafe way (ie from another thread) \$\endgroup\$ – Dan Pantry Sep 8 '14 at 13:21
  • \$\begingroup\$ I'm a bit lost. If Multicast delegates are immutable, according to my source, why is that not a guarantee of immutability? In what way could an event handler be a value type anyway? \$\endgroup\$ – Nick Udell Sep 8 '14 at 13:28
  • 2
    \$\begingroup\$ I feel I cannot explain it well enough in a comment, may I point you to the "Copy That!" section of this site to explain how this works? geekswithblogs.net/BlackRabbitCoder/archive/2011/12/01/… \$\endgroup\$ – Nick Udell Sep 8 '14 at 13:34
6
\$\begingroup\$

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.

\$\endgroup\$
2
\$\begingroup\$

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();
    }
}
\$\endgroup\$
  • \$\begingroup\$ This is good, although new Thread.Start should be changed to Task.Run. Further, I feel that the method should not have any side effects.. i.e, running it twice should not mean it does nothing, but instead return a cached variable. (I am assuming that DoSomeAction would return some kind of datum) This can be implemented using Lazy<T> \$\endgroup\$ – Dan Pantry Sep 8 '14 at 14:26
  • \$\begingroup\$ I am forced to use .NET 3.5 and therefore still think in 3.5. The whole should probably be changed to async (or Tasks) and there is nothing bad about spawning and releasing some class unless we really need to preserve the state. ...and about the rest (comment edited), I am just guessing what is the purpose. \$\endgroup\$ – firda Sep 8 '14 at 14:30
  • \$\begingroup\$ I am also guessing, I would hope the method is doing something with data so that's why I suggested the lazy. Must suck being stuck with 3.5 :\ \$\endgroup\$ – Dan Pantry Sep 8 '14 at 14:34

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.