9

I've got the following class:

public class Terminal : IDisposable
{
    readonly List<IListener> _listeners;

    public Terminal(IEnumerable<IListener> listeners)
    {
        _listeners = new List<IListener>(listeners);
    }

    public void Subscribe(ref Action<string> source)
    {
        source += Broadcast;
        //Store the reference somehow?
    }

    void Broadcast(string message)
    {
        foreach (var listener in _listeners) listener.Listen(message);
    }

    public void Dispose()
    {
        //Unsubscribe from all the stored sources?
    }
}

I've searched for a while and it appears that an argument passed with the ref keyword can't be stored. Trying to add the source argument to a list or to assign it to a field variable doesn't allow it to keep a reference to the actual delegate's original reference; so my questions are:

  • Is there a way to unsubscribe from all the sources without passing their references again?
  • If not, how can the class be changed in order to support it, but still maintain the subscription by passing a delegate through a method?
  • Is it possible to achieve it without using Reflection?
  • Is it possible to achieve it without wrapping the delegate/event in a class and then passing the class as a parameter for the subscription?

Thank you.

EDIT: It appears that without using a Wrapper or Reflection, there's no solution to the given problem. My intention was to make the class as much portable as possible, without having to wrap delegates in helper classes. Thanks everyone for the contributions.

svick
  • 236,525
  • 50
  • 385
  • 514
user1098567
  • 339
  • 3
  • 9
  • Would using an event with a custom `add` / `remove` handler work better for you? – vcsjones Dec 14 '11 at 20:44
  • It'd be easier to implement a Wrapper class then, because I have a lot of events and write custom implementations for them would make the code quite unreadable. My purpose is to make the class as much compatible as I can. – user1098567 Dec 14 '11 at 21:09
  • My example doesn't use a wrapper, even though it does require a little more extra work (creating/passing two delegates instead of one delegate ref). I've noticed the RX Framework does [something similiar](http://msdn.microsoft.com/en-us/library/hh211795(v=vs.103).aspx). It reminded me of this question and my answer. Probably worth looking at for your solution. – Mike McFarland Feb 09 '13 at 00:34

6 Answers6

2

Edit: Ok, that was a bad idea, so back to the basics:

I recommend creating a wrapper class over an Action:

class ActionWrapper
{
    public Action<string> Action;
}

And restructuring your initial class to work with wrappers:

private ActionWrapper localSource;

public void Subscribe(ActionWrapper source)
{
    source.Action += Broadcast;
    localSource = source;        
}

public void Dispose()
{
    localSource.Action -= Broadcast;
}

Now you should get the desired results.

Tudor
  • 61,523
  • 12
  • 102
  • 142
  • @Mike Christensen: Yeah, I probably misunderstood. Edited now. – Tudor Dec 14 '11 at 20:08
  • This will not work. Once stored in a field or in a list, the reference to the actual delegate is not stored. I've just tested it to be sure and in fact it doesn't unsubscribe successfully. – user1098567 Dec 14 '11 at 20:12
  • Yeah I think the poster was saying that didn't work (arguments passed by ref can't be stored?) - Perhaps the poster can clarify what they mean. – Mike Christensen Dec 14 '11 at 20:13
  • Is there a reason why you must pass by ref? – Mike Christensen Dec 14 '11 at 20:13
  • If the delegate is not passed by ref, the subscribed method will not be added to the invocation list of the given delegate. I suppose it creates a new instance of the delegate which is not bound anyhow to the original handler. – user1098567 Dec 14 '11 at 20:17
  • @user1098567: I've made an edit which I think is the solution. – Tudor Dec 14 '11 at 20:32
  • Thank you. That would definitely work sweetly, but I was wondering, as mentioned in the fourth point of the question (which I edited in a moment later, I apologize if it was unseen), if it was possible to do it without wrapping the event/delegate in a helper class. – user1098567 Dec 14 '11 at 20:39
  • Wonder if you can do something completely funky, such as write an implicit conversion between `ActionWrapper` and `Delegate`.. – Mike Christensen Dec 14 '11 at 21:07
  • @user1098567: I'm afraid that without a wrapper it's not possible. If you don't use ref, the objects are only passed by value, so you cannot modify them. If you use ref, they cannot be stored, as you said. So the only way to make a "ref that can also be stored" is to use a wrapper. – Tudor Dec 14 '11 at 21:08
  • 1
    @user1098567, remember that using the solution above will make Terminal hold a reference to ActionWrapper, which won't be collected and can generate memory leaks. – Bruno Brant Dec 15 '11 at 14:26
0
public class Terminal : IDisposable
{
  List<IListener> _listeners;
  List<Action<string>> _sources;

  public Terminal(IEnumerable<IListener> listeners)
  {
      _listeners = new List<IListener>(listeners);
      _sources = new List<Action<string>>();
  }

  public void Subscribe(ref Action<string> source)
  {
      _sources.Add( source );
      source += Broadcast;
  }

  void Broadcast(string message)
  {
      foreach (var listener in _listeners) listener.Listen(message);
  }

  public void Dispose()
  {
      foreach ( var s in _sources ) s -= Broadcast; 
  }
}
Wiktor Zychla
  • 47,367
  • 6
  • 74
  • 106
0

EDIT:

Yep, my bad - delegates are immutable types, so adding a method to an invocation list will actually create a new delegate instance.

Which leads to an answer no to your question. To unsubscribe the delegate you need to remove your Broadcast method from the delegate's invocation list. This means creating a new delegate and assigning it to the original field or variable. But you cannot access the original once you're out of Subscribe method. Plus there can be other copies of that original field/variable that have your method on the invocation list. And there is no way for you to know about all of them and change there values.

I'd suggest to declare an interface with the event for your purpose. This will be quite flexible approach.

public interface IMessageSource
{
    event Action<string> OnMessage;
}

public class MessageSource : IMessageSource
{
    public event Action<string> OnMessage;

    public void Send(string m)
    {
        if (OnMessage!= null) OnMessage(m);
    }
}

public class Terminal : IDisposable
{
    private IList<IMessageSource> sources = new List<IMessageSource>();

    public void Subscribe(IMessageSource source)
    {
        source.OnMessage += Broadcast;
        sources.Add(source);
    }


    void Broadcast(string message)
    {
        Console.WriteLine(message);
    }

    public void Dispose()
    {
        foreach (var s in sources) s.OnMessage -= Broadcast;
    }
}

Original answer

Is there a particular reason why you pass source delegate as ref? You need this if you, for example, want to return a different delegate from the method.

Otherwise, the delegate is reference type, so you can subscribe to it without passing it as ref...

AlexD
  • 5,011
  • 2
  • 23
  • 34
  • 2
    Unfortunately no, you can test it too; if you don't pass the delegate's instance by ref, subscribing to it will not work. – user1098567 Dec 14 '11 at 20:23
0

I would suggest that the subscription method should return an implementation of a SubscriptionHelper class, which implements IDisposable. A simple implementation would be for SubscriptionHelper to hold a reference to the subscription list and a copy of the subscription delegate; the subscription list itself would be a List<SubscriptionHelper>, and the Dispose method for SubscriptionHelper would remove itself from the list. Note that if the same delegate gets subscribed multiple times, each subscription will return a different SubscriptionHelper; calling Dispose on a SubscriptionHelper will cancel the subscription for which it had been returned.

Such an approach would be much cleaner than the Delegate.Combine/Delegate.Remove method used by the normal .net pattern, whose semantics can get very strange if an attempt is made to subscribe and unsubscribe multi-target delegates.

supercat
  • 77,689
  • 9
  • 166
  • 211
  • I apologize, but I'm afraid that I don't understand: the critical part for me would be keeping a reference to the original delegate in order to unsubscribe from it (thus modifying its original subscription list). How would the SubscriptionHelper hold a valid reference to the given delegate? – user1098567 Dec 14 '11 at 20:31
  • @user1098567: The constructor for the SubscriptionHelper would take the delegate as a parameter. Since multiple subscriptions using the same delegate would generate distinct SubscriptionHelper objects, and since the subscription list would be a list of SubscriptionHelpers rather than delegates, calling Dispose on the object returned by second subscription request using a particular delegate would cancel the second subscription made using that delegate, even the delegate had been subscribed more times after the second subscription request. – supercat Dec 14 '11 at 20:42
0

It's reasonably simple, but there are a few pitfalls. If you store a reference to the source objects, as most of the examples so far have proposed, the object won't be garbage collected. The best way to avoid this is to use an WeakReference, that will allow the GC to work properly.

So, all you have to do is this:

1) Add a list of sources to the class:

private readonly List<WeakReference> _sources = new List<WeakReference>();

2) Add the source to the list:

public void Subscribe(ref Action<string> source)
{
    source += Broadcast;
    //Store the reference 
    _sources.Add(new WeakReference(source));
}

3) And then just implement dispose:

public void Dispose()
{
    foreach (var r in _sources)
    {
        var source = (Action<string>) r.Target;
        if (source != null) 
        {
            source -= Broadcast;
            source = null;
        }
    }


    _sources.Clear();
}

That said, there's also the question of why the Action must be passed as a ref. In the current code, there's no reason for that. Anyway, it doesn't affect the problem or the solution.

Bruno Brant
  • 8,226
  • 7
  • 45
  • 90
  • Hi, unfortunately it does make a difference if the Action is passed as a ref. If you pass it as a ref, it will subscribe successfully but using your method, won't unsubscribe. By passing it with no ref keyword, it will just not subscribe. – user1098567 Dec 14 '11 at 22:23
  • @user1098567, after reading the other answer and making some tests I understood the problem more clearly. Action isn't immutable (otherwise not even passing by ref would work), but it's a ValueType and as such, is unable to be cloned. It was indeed a nice question, made me think about things. Just be careful when subscribing and use weak references to be safe against memory leaks. – Bruno Brant Dec 15 '11 at 14:27
0

Perhaps, instead of trying to store a reference to the delegate, have what calls Subscribe use its reference to the object with the delegate to create actions for the subscription and unsubscription . Its an additional parameter, but its still straightforward.

public void Subscribe(Action<Action<string>> addHandler,Action<Action<string>> removeHandler)
    {
        //Prevent error for possibly being null in closure
        Action<string> onEvent = delegate { };

        //Broadcast when the event occurs, unlisten after (you could store onEvent and remove handler yourself)
        onEvent = (s) => { Broadcast(s); removeHandler(onEvent); };
        addHandler(onEvent);
    }

And an example subscribing.

public event Action<string> CallOccured;

    public void Program()
    {
        Subscribe(a => CallOccured += a, a => CallOccured -= a);
        CallOccured("Hello");
    }
Mike McFarland
  • 657
  • 4
  • 17