2

I have the following dictionary:

private Dictionary<string, Action<GameObject, int, int, int, int>> eventDictionary;

I wish to keep a dictionary of Actions (basically delegates) so that whenever I wish to subscribe to an event name, I can subscribe to the same event for all my subscribers.

This is my function to listen to a certain event string:

public static void StartListening(string eventName, Action<GameObject, int, int, int, int> listener)
{
    Action<GameObject, int, int, int, int> thisEvent = null;
    if (instance.eventDictionary.TryGetValue(eventName, out thisEvent))
    {
        thisEvent += listener;
        // the code reaches this 
    }
    else
    {
        thisEvent += listener;
        instance.eventDictionary.Add(eventName, thisEvent);
    }
}

Now I try 

EventManager.StartListening("Move", Moved);
EventManager.StartListening("Move", Moved);
EventManager.StartListening("Move", Moved);
EventManager.StartListening("Move", Moved);

// Log here to get how many subscribers there are to the event "Move"
// Result: only 1 listener

Only the first added listener will actually register, the rest "disappear" after adding them. I debugged this error for nearly 4 hours, before finally testing to see if maybe the line thisEvent += listener; was malfunctioning. When I added a remove and subsequent add back to the dictionary, 

public static void StartListening(string eventName, Action<GameObject, int, int, int, int> listener)
{
    Action<GameObject, int, int, int, int> thisEvent = null;
    if (instance.eventDictionary.TryGetValue(eventName, out thisEvent))
    {
        thisEvent += listener;
        instance.eventDictionary.Remove(eventName);
        instance.eventDictionary.Add(eventName, thisEvent);
    }
    else
    {
        thisEvent += listener;
        instance.eventDictionary.Add(eventName, thisEvent);
    }
}

the delegates finally got added as expected.

EventManager.StartListening("Move", Moved);
EventManager.StartListening("Move", Moved);
EventManager.StartListening("Move", Moved);
EventManager.StartListening("Move", Moved);

// Log here to get how many subscribers there are to the event "Move"
// Result: 4 listeners

This is one of the most nonsensical errors I have ever gotten. Aren't all values in a dictionary that aren't strings, ints, etc. supposed to be retrieved by reference, not by value? Why do I get a clone of the Action here, rather than a reference?

PS: GameObject is a Unity class. This is my Moved function: 

public void Moved(GameObject invoker, int x, int z, int tx, int tz)
{
    //Some code here
}
ProgrammingLlama
  • 36,677
  • 7
  • 67
  • 86
TheForgot3n1
  • 212
  • 2
  • 11
  • [Related](https://stackoverflow.com/questions/35968889/what-is-the-purpose-of-the-delegates-are-immutable-in-c) – ProgrammingLlama Dec 24 '19 at 05:45
  • What would you expect to happen in your else statement since thisEvent is most likely null (if it has a class type) – Joachim Isaksson Dec 24 '19 at 05:46
  • Also you might be surprised to learn that `string` is an "immutable" reference type. – ProgrammingLlama Dec 24 '19 at 05:46
  • I expect the else statement to run once (which it does), in order to create the dictionary entry. – TheForgot3n1 Dec 24 '19 at 05:55
  • I believe that the += operator will create a new instance before setting it back to `thisEvent`, so you need to update the dictionary entry as well, like this `instance.eventDictionary[eventName] = thisEvent;` – Hopeless Dec 24 '19 at 09:13

1 Answers1

3

This is one of the most nonsensical errors I have ever gotten. Aren't all values in a dictionary that aren't strings, ints, etc. supposed to be retrieved by reference, not by value? Why do I get a clone of the Action here, rather than a reference?

When you call TryGetValue(eventName, out thisEvent) you are providing a reference to which the Dictionary will write the value. You are not getting a reference to what is inside the Dictionary (I mean, you are not getting a deep pointer to the Dictionary structure, meaning that assigning to it will NOT modify the Dictionary).

Perhaps some confusion might stem from the fact that a delegate is a reference type. And yes, you get a reference to the same delegate object, not a new one. However, delegate addition returns a new multicast delegate.

By the way, string is a reference type.

See:

I'm just left wondering why it is designed that way. Almost all languages have dictionaries that give you back a reference when getting objects. How else should I get objects when I am not sure if it is null?

Memory safety. There will be no chance of orphan pointers or similar with this design.

Although, it can be considered an oversight not to have an alternative API for updates, such as ConcurrentDictionary<TKey,TValue>.AddOrUpdate. Dictionary is not meant to be thread safe, and you can accomplish the same thing, even if less efficiently... that would explain why it was not considered necesary.

If you are looking for an alternative (other than ConcurrentDictionary) you can use a mutable reference type. Such as List<Delegate> for example. In TryGetValue(eventName, out thisEvent) you would still be providing a reference to which to write the List<Delegate>, however, you can then mutate it. However, you still have to deal with initializing it, which you would do when the key is not present. You would not have nulls.

if (instance.eventDictionary.TryGetValue(eventName, out var thisEvent))
{
    thisEvent.Add(listener);
}
else
{
    instance.eventDictionary.Add
    (
        eventName,
        new List<Action<GameObject, int, int, int, int> {listener}
    );
}

This also makes me wonder, is it a deep or shallow clone?

If you are storing a value type, you get a copy of the value type.

If you are storing a reference type, you get copy of the reference. I would not call that cloning. It is just another reference to the same object.

Theraot
  • 31,890
  • 5
  • 57
  • 86
  • What you're saying makes sense, but I'm just left wondering why it is designed that way. Almost all languages have dictionaries that give you back a reference when getting objects. How else should I get objects when I am not sure if it is null? This also makes me wonder, is it a deep or shallow clone? – TheForgot3n1 Dec 24 '19 at 05:54
  • 1
    In normal C#, I get a reference to the item stored in the dictionary... https://dotnetfiddle.net/1zc8rr – ps2goat Dec 24 '19 at 05:59
  • @ps2goat this is not abnormal C#, simply the delegate is not mutable, while the `List` is mutable. In both cases you get a reference to the object, but not a reference to the storage in the dictionary (as in, setting to it will not replace the contents of the dictionary). See: https://dotnetfiddle.net/MbjGJH (compare with using `+=` on the delegate, we are doing assignment, in case that is not clear... you can write `thisEvent += listener` like this `thisEvent = thisEvent + listener`). – Theraot Dec 24 '19 at 06:14
  • 1
    Yeah. Theraot is correct. It works totally fine if u dont use TryGetValue... https://dotnetfiddle.net/nGCLt1 – TheForgot3n1 Dec 24 '19 at 06:20
  • Ok. At least with Unity 5, it used dotnet when running on a Windows machine locally, and mono when not on Windows. So I consider that to be a possibility for `abnormal`ness. – ps2goat Dec 24 '19 at 06:32
  • Thank you so much for the great answer, Theraot. So quick and thorough. I almost feel like I should have debugged less and just asked here haha – TheForgot3n1 Dec 24 '19 at 06:35
  • @ps2goat I think it makes sense, given Theraot's answer. When I do my reassignment (`thisEvent = thisEvent + listener`) all I am doing is just changing what my local variable points to, not what the dictionary value points to. – TheForgot3n1 Dec 24 '19 at 06:37
  • That's fine, I'm just scraping some knowledge for myself in the process. I definitely agree with the approach to use a List or something similar to store multiple things. In my initial review of the code, I thought you were simply adding listeners to an event, which isn't the case. – ps2goat Dec 24 '19 at 06:38
  • @TheForgot3n1 With modern C#, it could be possible to have an API that returns a reference such that assigning to it modifies the internal structure. And without unsafe code. See [Ref returns and ref locals](https://learn.microsoft.com/en-us/dotnet/csharp/programming-guide/classes-and-structs/ref-returns). There are restrictions around it, so that the compiler can ensure that what is being referenced is available for the scope. (to be continued...) – Theraot Dec 24 '19 at 06:52
  • @TheForgot3n1 (...continuation) Although that what is being referenced stays available does not mean that it will continue to be part of the structure. For example, if we are talking of a tree and a branch is removed, your reference could point to something that is no longer in the tree you think it is. Also, if you are getting a reference to an array, and the code moves values in it (to keep them compact, for example), then you may end with a reference to a different value. I would rather get a callback with a ref with the guarantee that it stays valid for the callback, than ref return. – Theraot Dec 24 '19 at 06:55
  • @theraot I had no idea you could do that. Good article link! Yeah regular references usually suffice, but this is good to know about. – TheForgot3n1 Dec 24 '19 at 07:10