Memoryleak in TObjectDataSet's DoPostRecord method

[Adam Siwoń]

I found small memoryleak problem in TObjectDataSet's DoPostRecord method. When while posting inserted record to a dataset occurs any exception during setting property value, then newItem instance created for assignment data is not freed and this cause memory leak. It is because newItem is created but not assigned to the list what could ensure proper instance destroying. I think DoPostRecord method should looks like:

procedure TObjectDataSet.DoPostRecord(Index: Integer; Append: Boolean);
var
  newItem: TObject;
  freeOnException: Boolean;
  sortNeeded: Boolean;
  i: Integer;
  field: TField;
  fieldValue: Variant;
  value: TValue;
  prop: TRttiProperty;
begin
  freeOnException := State = dsInsert;
  if State = dsInsert then
    newItem := TActivator.CreateInstance(fItemTypeInfo)
  else
    newItem := IndexList.Items[Index];
  try
    sortNeeded := False;

    for i := 0 to ModifiedFields.Count - 1 do
    begin
      field := ModifiedFields[i];

      if not sortNeeded and Sorted then
        sortNeeded := FieldInSortIndex(field);

      // Fields not found in dictionary are calculated or lookup fields, do not post them
      if fProperties.TryGetFirst(prop, TPropertyFilters.IsNamed(field.FieldName)) then
      begin
        fieldValue := field.Value;
        if VarIsNull(fieldValue) then
          prop.SetValue(newItem, TValue.Empty)
        else
          if TValue.From<Variant>(fieldValue).TryConvert(prop.PropertyType.Handle, value) then
            prop.SetValue(newItem, value);  -- here an exception in property setting can be thrown...
      end;
    end;

    if State = dsInsert then
      if Append then
        Index := IndexList.AddItem(newItem)
      else
        IndexList.InsertItem(newItem, Index);
  except     -- freeing newItem instance when something went wrong
    if freeOnException then
      newItem.Free;
    raise;
  end;
...

here is added try except block which allows to free item not assigned to the list when something went wrong during data assignment to the item or list.

I found this problem in Branch_2.0-rc.3 but probably exists also in older versions.

[Stefan Glienke]

fixed #394

→ <<cset 551daecd14a8>>

[Adam Siwoń]

Thank you very much for correcting this. Don’t you think the code with adding item to the list also should be in try except section? Only when an item is successfully added to the list we can be sure that there will be no memory leak. While adding item to the list also could be an exception raised. Maybe I'm paranoid :-), but it happens sometimes.

[Stefan Glienke]

You could have an event on the list that raises an exception - but at that point the item was already added to the list and must not be freed.

There could be that rare case of an exception happening when adding the item to the list but before it was successfully added but to be fair, if that happens you are in more serious trouble than leaking that object.