'Access violation' in TDuckTypedList.CanBeWrappedAsList (line 169) about delphi-event-bus HOT 15 CLOSED

from delphi-event-bus.

@spinettaro,
I'll see what I can do to provide an example to reproduce this error.

I found the source of TObjectClone: Object cloning using the high level RTTI

IIRC, I switched to TObjectClone because few years ago RTTIUtils had issues handling circular object reference, we discussed it here. However, I just run the EventBus tests with RTTIUtils replaced by TObjectClone it emits two errors shown below.

So although I'm having an unknown issue with RTTIUtils as reported in the original post, I'm not sure which one is more stable...

DUnitX - [DEBDUnitXTests.exe] - Starting Tests.

.........................E...F..

Tests Found   : 16
Tests Ignored : 0
Tests Passed  : 14
Tests Leaked  : 0
Tests Failed  : 1
Tests Errored : 1

Failing Tests
  EventBusTestU.TEventBusTest.TestPostEntityWithObjectList
  Message: Expected 2 but got 0

Tests With Errors
  EventBusTestU.TEventBusTest.TestPostEntityWithChildObject
  Message: Invalid pointer operation

from delphi-event-bus.

@edwinyzh , yes the problem is that TObjectClone doesn't make a deep serialization. What I mean is that if you have an object with a property that is another object ( also a list ) this is not serialized. TObjectClone maybe is good because is a bad practice to pass a complex event (an object with other objects as children) and I want to avoid this. But some developer use a complex event. What do you think about?

from delphi-event-bus.

@spinettaro, I think you have raised a very good question. Maybe it's time to re-think the 'event cloning' logic that happens before invoking each subscriber method.

There are two types of fields/properties of an object:

• Pass-by-value types, such as string, integer, record, and so on. these field/property values should be copied.
• Pass-by-reference types, such as object and interface, they are basically pointers. These values should NOT be deep-copied - we should just assign the object/interface reference to the target event object. I think this is more logical.

Actually I haven't looked at the implementation details of TObjectClone nor RTTIUtils, I wish you I described it clear.

Does it make sense?

from delphi-event-bus.

@edwinyzh yes sure, but this is the dilemma:

• DEB uses TObject or descendants to represent an event
• Pass by value is not possible by now, otherwise we have to change this point and make an event a simple type (maybe the best one is a string)
• So I pass an event by reference, but who is responsible for free that event? Because DEB uses also asynchronous calls and you could free an event while a subscribe is not yet notified. This is the reason because I clone the event, so every subscriber is responsible for event memory deallocation, and every subscriber is sure that event object is not freed from someone. Does it make sense? Could you suggest other solutions/scenario?

from delphi-event-bus.

@spinettaro,
I think there is a misunderstanding here, and maybe a misunderstanding on my side too, because I haven't reviewed the 'event object cloning' code yet, I'll do that later.

I don't mean not to duplicate the event object, I think your current design is OK - the event is a derived class of TObject, that's very flexible, and we certainly can't change that, otherwise it'll break backward compatibility.

I'll need to review the event object cloning code before I can discuss further in this direction. But I'll focus on reproducing the original reported issue first.

from delphi-event-bus.

Shallow clone seems ok for me and the documentation should notice event objects should not own any other objects, just hold references to them. I think this is very reasonable.
One alternative solution if the deep clone is chosen would be to use annotation to indicate sub-objects that are not owned by the event object or should not be cloned for any other reason. These would be annotated with [DO_NOT_CLONE]. I prefer the shallow clone solution.

I also noticed the clone method was not checking for nil in some places and changed it accordingly (see below). That was the reason for some exceptions I was getting. I did not prepared a pull request because I want to test the TObjectClone option.

class procedure TRTTIUtils.CopyObject(SourceObj, TargetObj: TObject);
var
  _ARttiType: TRttiType;
  Field: TRttiField;
  master, cloned: TObject;
  Src: TObject;
  sourceStream: TStream;
  SavedPosition: Int64;
  targetStream: TStream;
  targetCollection: IWrappedList;
  sourceCollection: IWrappedList;
  I: Integer;
  sourceObject: TObject;
  targetObject: TObject;
  Tar: TObject;
begin
  if not Assigned(TargetObj) then
    Exit;

  _ARttiType := ctx.GetType(SourceObj.ClassType);
  cloned := TargetObj;
  master := SourceObj;
  for Field in _ARttiType.GetFields do
  begin
    if not Field.FieldType.IsInstance then
      Field.SetValue(cloned, Field.GetValue(master))
    else
    begin
      Src := Field.GetValue(SourceObj).AsObject;
      if Assigned (Src) then
      begin
        if Src is TStream then
        begin
          sourceStream := TStream(Src);
          SavedPosition := sourceStream.Position;
          sourceStream.Position := 0;
          if Field.GetValue(cloned).IsEmpty then
          begin
            targetStream := TMemoryStream.Create;
            Field.SetValue(cloned, targetStream);
          end
          else
            targetStream := Field.GetValue(cloned).AsObject as TStream;
          targetStream.Position := 0;
          targetStream.CopyFrom(sourceStream, sourceStream.Size);
          targetStream.Position := SavedPosition;
          sourceStream.Position := SavedPosition;
        end
        else if TDuckTypedList.CanBeWrappedAsList(Src) then
        begin
          sourceCollection := WrapAsList(Src);
          Tar := Field.GetValue(cloned).AsObject;
          if Assigned(Tar) then
          begin
            targetCollection := WrapAsList(Tar);
            targetCollection.Clear;
            for I := 0 to sourceCollection.Count - 1 do
              targetCollection.Add(TRTTIUtils.Clone(sourceCollection.GetItem(I)));
          end;
        end
        else
        begin
          sourceObject := Src;

          if Field.GetValue(cloned).IsEmpty then
          begin
            targetObject := TRTTIUtils.Clone(sourceObject);
            Field.SetValue(cloned, targetObject);
          end
          else
          begin
            targetObject := Field.GetValue(cloned).AsObject;
            TRTTIUtils.CopyObject(sourceObject, targetObject);
          end;
        end;
      end
      else
        targetObject := nil;
    end;
  end;
end;

class function TRTTIUtils.Clone(Obj: TObject): TObject;
var
  _ARttiType: TRttiType;
  Field: TRttiField;
  master, cloned: TObject;
  Src: TObject;
  sourceStream: TStream;
  SavedPosition: Int64;
  targetStream: TStream;
  targetCollection: TObjectList<TObject>;
  sourceCollection: TObjectList<TObject>;
  I: Integer;
  sourceObject: TObject;
  targetObject: TObject;
begin
  Result := nil;
  if not Assigned(Obj) then
    Exit;

  _ARttiType := ctx.GetType(Obj.ClassType);
  cloned := CreateObject(_ARttiType);
  master := Obj;
  for Field in _ARttiType.GetFields do
  begin
    if not Field.FieldType.IsInstance then
      Field.SetValue(cloned, Field.GetValue(master))
    else
    begin
      Src := Field.GetValue(Obj).AsObject;
      if Src is TStream then
      begin
        sourceStream := TStream(Src);
        SavedPosition := sourceStream.Position;
        sourceStream.Position := 0;
        if Field.GetValue(cloned).IsEmpty then
        begin
          targetStream := TMemoryStream.Create;
          Field.SetValue(cloned, targetStream);
        end
        else
          targetStream := Field.GetValue(cloned).AsObject as TStream;
        targetStream.Position := 0;
        targetStream.CopyFrom(sourceStream, sourceStream.Size);
        targetStream.Position := SavedPosition;
        sourceStream.Position := SavedPosition;
      end
      else if Src is TObjectList<TObject> then
      begin
        sourceCollection := TObjectList<TObject>(Src);
        if Field.GetValue(cloned).IsEmpty then
        begin
          targetCollection := TObjectList<TObject>.Create;
          Field.SetValue(cloned, targetCollection);
        end
        else
          targetCollection := Field.GetValue(cloned).AsObject as TObjectList<TObject>;
        for I := 0 to sourceCollection.Count - 1 do
        begin
          targetCollection.Add(TRTTIUtils.Clone(sourceCollection[I]));
        end;
      end
      else
      begin
        sourceObject := Src;
        if Assigned (sourceObject) then
        begin
          if Field.GetValue(cloned).IsEmpty then
          begin
            targetObject := TRTTIUtils.Clone(sourceObject);
            Field.SetValue(cloned, targetObject);
          end
          else
          begin
            targetObject := Field.GetValue(cloned).AsObject;
            TRTTIUtils.CopyObject(sourceObject, targetObject);
          end;
          Field.SetValue(cloned, targetObject);
        end
        else
          Field.SetValue(cloned, nil);
      end;
    end;

  end;
  Result := cloned;
end;

from delphi-event-bus.

Thanks for sharing. In this pull request I changed the TEvent.CloneEvent method virtual so I can override the behavior.

from delphi-event-bus.

Thanks for PR @edwinyzh ! @mgpensar yes, my thought was to allow a mechanism that is a bit more flexible of simple String in Event data. This is why you can pass an object, but if you try to pass a complex object maybe this is a synthom that there is something wrong in your architecture because it is, in the most of case, an antipattern. This is why, in my opinion, an Event should be only a simple object.

from delphi-event-bus.

The built-in TRTTIUtils.Clone is really not very stable, aside from the above discussions, I had experiece with the 'stack overflow' errors.

I agree that an Event should use only simple objects, and I also agree with @mgpensar about we should use shallow copy for the event objects.

Actually, IMHO the best option would be use System.TypInfo.pas, AKA the good-old, fast RTTI, which means only published event properties will be copied, this is a tiny rule and I think it's perfectly OK. mORMot had a great performance success with the good old, fast RTTI, check CopyObject in mORMot.pas for an idea.

But the bottom-line is, we should not break backward compatibility, so maybe implement a new TEvent.PostEx :)

from delphi-event-bus.

Hi guys, @edwinyzh @mgpensar , what about to create a service inside DEB that clone the object? You can also specify your custom cloning for a specific type otherwise the stndard one (RTTI) is used or you can also inject your clone service... What are your thoughts?

from delphi-event-bus.

Good idea! To simplify things, maybe a single call back is enough, something like
TOnCloneEventObject = procedure (aEventObject: TObject) of TObject
And the client code to use it:
TEventBus.Default.EventObjectCloneProc = ...

from delphi-event-bus.

added mechanism to customize event cloning. See last commit and unit tests to understand how it works.

from delphi-event-bus.

Thanks! I see you took the anonymous method approach, not sure if how it performs comparing with the traditional method call back.

from delphi-event-bus.

@edwinyzh changed with TCloneEventCallback (function of object) and TCloneEventMethod (reference to function)

from delphi-event-bus.