Adding the same Object twice to a TObjectDictionary frees the object

Question

Look at this code:

dic:=TObjectDictionary<Integer, TObject>.Create([doOwnsValues]);
testObject:=TObject.Create;
dic.AddOrSetValue(1,testObject);
dic.AddOrSetValue(1,testObject);

The code

1. Creates a Dictionary that owns the contained values
2. Adds a value
3. Adds the same value again, using the same key

The surprising thing is that the object is freed when you add it the second time.

Is this intended behaviour? Or a bug in the Delphi libraries?

The documentation simply says "If the object is owned, when the entry is removed from the dictionary, the key and/or value is freed". So it seems a little odd to Free an object that I have just asked it to Add!

Is there any way to tell the TObjectDictionary to not do this? Currently, each time I add a value I have to check first if that Key-Value combination is already in the Dictionary.

Delphi 2010

[EDIT: After reading all the comments:

My conclusions (for what they are worth)]

• This seems to be the intended behaviour
• There is no way of modifying this behaviour
• Don't use TObjectDictionary (or any of the other similar classes) for anything other than the common "Add these objects to the container. Leave them there. Do some stuff. Free the container and all the objects you added" usage. If you are doing anything more complicated, it's better to manage the objects yourself.
• The behaviour is poorly documented and you should read the source if you want to really know what's going on

[/EDIT]

Answer 1

TObjectDictionary<TKey,TValue> is in fact just a TDictionary<TKey,TValue> which has some extra code in the KeyNotify and ValueNotify methods:

procedure TObjectDictionary<TKey,TValue>.ValueNotify(const Value: TValue; 
  Action: TCollectionNotification);
begin
  inherited;
  if (Action = cnRemoved) and (doOwnsValues in FOwnerships) then
    PObject(@Value)^.Free;
end;

This is, IMO, a rather simple minded approach, but in the ValueNotify method, it is impossible to tell for which key this is, so it simply frees the "old" value (there is no way to check if this value is set for the same key).

You can either write your own class (which is not trivial), deriving from TDictionary<K,V>, or simply not use doOwnsValues. You can also write a simple wrapper, e.g. TValueOwningDictionary<K,V> that uses TDictionary<K,V> to do the brunt of the work, but handles the ownership issues itself. I guess I would do the latter.

Answer 2

Thats because with reusing the key youre replacing the object and since the dictionary owns the object it frees the old one. Dictionary doesn't compare the value, only key, so it doesn't detect that the value (object) is same. Not a bug, as designed (IOW user error).

On second thought - perhaps the designer of the dict should have taken more care to have both doOwnsValues and AddOrSetValue()... one can argue both ways... I suggest you file it in QC, but I wouldn't hold my breath - it has been so now in at least two releases so it's unlikely to change.

Answer 3

This behaviour is by design and the design is sound.

Were the class to take responsibility for not freeing duplicates, it would have to iterate over the whole container every time a modification was made, both adding and removing. The iteration would check for any duplicate values and check accordingly.

It would be disasterous to impose this diabolical performance drain on all users of the class. If you wish to put duplicates in the list then you will have to come up with a bespoke lifetime management policy that suits your specific needs. In this case it is unreasonable to expect the general purpose container to support your particular usage pattern.

---

In the comments to this answer, and many of the others, it has been suggested that a better design would have been to test in AddOrSetValue whether or not the value being set was already assigned to the specified key. If so, then AddOrSetValue could return immediately.

I think it's clear to anyone that checking for duplicates in full generality is too expensive to contemplate. However, I contend that there are good design reasons why checking for duplicate K and V in AddOrSetValue would also be poor design.

Remember that TObjectDictionary<K,V> is derived from TDictionary<K,V>. For the more general class, comparing equality of V is potentially an expensive operation because we have no constraints on what V is, it being generic. So for TDictionary<K,V> there are performance reasons why we should not include the putative AddOrSetValue test.

It could be argued that we make a special exception for TObjectDictionary<K,V>. That would certainly be possible. It would require a little re-engineering of the coupling between the two classes, but it is quite feasible. But now you have a situation where TDictionary<K,V> and TObjectDictionary<K,V> have different semantics. This is a clear downside and must be weighed against the potential benefit from the AddOrSetValue test.

These generic container classes are so fundamental that design decisions have to take into account a huge spread of use cases, consistency considerations and so on. It is not, in my view, reasonable to consider TObjectDictionary<K,V>.AddOrSetValue in isolation.

Answer 4

Since the Delphi TDictionary implementation doesn't allow for more than one of the same keys you could check the excellent Generic collections library from Alex Ciobanu. It comes with a TMultiMap or for your case TObjectMultiMap that allows for multiple values per key.

Edit: If you don't want multiple values per key, but rather want to avoid adding duplicates to the Dictionary then you can try TDistinctMultiMap or a TObjectDistinctMultiMap from the same Collections library.

Answer 5

So it seems a little odd to Free an object that I have just asked it to Add!

You didn't ask the dictionary to add - you called 'AddorSet', and since the key was already found, your call was a 'set', not an 'add'. Regardless, I see nothing odd here in terms of Delphi's behavior: In Delphi, objects are only object references, and there is no reference counting or ownership for simple objects.

Since in this case the dictionary owns the objects, it is doing exactly what it's supposed to do: "If the object is owned, when the entry is removed from the dictionary, the key and/or value is freed". You removed the value when you overwrote entry[1] - therefore the object referred to in 'testObject' is immediately deleted and your reference to 'testObject' is invalid.

Currently, each time I add a value I have to check first if it's already in the Dictionary.

Why is that? The behavior you described should only occur if you overwrite a previously used key with a reference to the same object.

---

Edit:

Perhaps there is something 'odd' after all - try this test code:

        procedure testObjectList;
        var ol:TObjectList;
            o,o1:TObject;
        begin

          ol:=TObjectList.create;
          ol.OwnsObjects:=true;//default behavior, not really necessary
          try
            o:=TObject.create;      
            ol.add(o);
            ol[0]:=o;
            showmessage(o.ClassName);//no av-although ol[0] is overwritten with o again, o is not deleted
            o1:=TObject.create;
            ol[0]:=o1;
            showmessage(o.ClassName);//av - when o is overwritten with o1, o is deleted
          finally
            ol.free
          end;

        end;

This in spite of what it says in the (Delphi 7) help: "TObjectList controls the memory of its objects, freeing an object when its index is reassigned"

Answer 6

I think it is a bug. I ran into it a week ago.

I use the TObjectDictionary for storing some real time telemetria datas, which are very often updated with new datas.

for example:

Type TTag = class
               updatetime : TDateTime;
               Value      : string ;
            end ;

TTagDictionary:= TObjectDictionary<string,TTag>.Create([doOwnsValues]);


procedure UpdateTags(key: string; newValue: String) ;
var 
   tag : TTag ;
begin
     if TTagDictionary.TryGetValue(key,tag) then begin  // update the stored tag
        tag.Value = newValue ;
        tag.updatetime := now ;
        TTagDictionary.AddorSetValue(key,tag) ;
     else begin
        tag := TTag.Create ;
        tag.updatetime := now ;
        tag.Vluae := newValue ;
        TTagDictionary.AddorSetValue(key,tag) ;
     end ;
end ;

After several updates I ended up with some nasty access violations and with an dictionary full of freed objects.

It is a very poor designed container.

At update it need check if the new object is the same as the old only and then it must NOT free the object.