mORMot Open Source

BBackSoon

Member

Registered: 2014-11-15

Posts: 41

ObjectToJSON and memory leaks?

I have found a strange memory leak that apparently affects the ObjectToJSON function. To confirm I have created a short program that compares the ObjectToJSON function with another commonly used object serializer (OmniXML). Even though one function writes a JSON string and the other one writes an XML string, they both do the same thing: they READ the object to be serialized and WRITE a string (regardless of the format of such string).

Here's the code:

unit Unit1;

interface

uses
  Winapi.Windows, Winapi.Messages, System.SysUtils, System.Variants, System.Classes, Vcl.Graphics,
  Vcl.Controls, Vcl.Forms, Vcl.Dialogs, Vcl.StdCtrls;

type
  TForm1 = class(TForm)
    Label1: TLabel;
    Label2: TLabel;
    Button1: TButton;
    Button2: TButton;
    procedure Button1Click(Sender: TObject);
    procedure Button2Click(Sender: TObject);
  private
    { Private declarations }
  public
    { Public declarations }
  end;

  TMyObj = class(TPersistent)
  private
    fSomething: TStringList;
    function GetSomething: TStringList;
    procedure SetSomething(Value: TStringList);
  public
    constructor Create;
    destructor Destroy; override;
  published
    property Something: TStringList read GetSomething write SetSomething;
  end;

var
  Form1: TForm1;

implementation

uses
  SynCommons, mORMot, OmniXML, OmniXMLPersistent, OmniXML_Types;

{$R *.dfm}

constructor TMyObj.Create;
begin
  inherited Create;
  fSomething := TStringList.Create;
  fSomething.Add('First string');
  fSomething.Add('Second string');
end;

destructor TMyObj.Destroy;
begin
  FreeAndNil(fSomething);
  inherited Destroy;
end;

function TMyObj.GetSomething: TStringList;
begin
  Result := fSomething;
end;

procedure TMyObj.SetSomething(Value: TStringList);
begin
  fSomething.Assign(Value);
end;

procedure TForm1.Button1Click(Sender: TObject);
var
  json: RawUTF8;
  i: integer;
  obj: TMyObj;
  res: boolean;
begin
  obj := TMyObj.Create;
  for i := 0 to 1999 do
  begin
    // mORMot serializer, requires to register TCollections descendants, and
    // leaks memory (even just with TStringList properties)
    json := ObjectToJSON(obj, [woHumanReadable]);
  end;
  FreeAndNil(obj);
  MessageDlg('Done. Now close the program to see 2000 memory leaks... one per each iteration of JSONToObject.', mtInformation, [mbOK], 0);
end;

procedure TForm1.Button2Click(Sender: TObject);
var
  obj: TMyObj;
  i: integer;
  xml: XmlString;
begin
  obj := TMyObj.Create;
  for i := 0 to 1999 do
  begin
    // OmniXML serialization causes no leaks... and it even works with TCollection child objects
    // without the need to register them...
    TOmniXMLWriter.SaveXML(obj, xml, pfAttributes, ofIndent);
  end;
  FreeAndNil(obj);
end;

end.

Now, one may argue that the property setter for the TStringList should be like this:

procedure TMyObj.SetSomething(Value: TStringList);
begin
  fSomething.Free;
  fSomething := Value;
end;

But everywhere in the Delphi code itself, and on pretty much all Delphi programming books that I've read, they use the non-destructive assign method:

procedure TMyObj.SetSomething(Value: TStringList);
begin
  fSomething.Assign(Value);
end;

On top of that, and on a more general scale, I can see that other serializers (like OmniXML) work like a charm with the Assign-based property setter, and don't leak memory. Also, as a side note, OmniXML serializes collections without the need to register them with the serializer (but that might be a side-effect of the greater verbosity of XML versus the data-centric nature of JSON).

BBackSoon

Member

Registered: 2014-11-15

Posts: 41

Re: ObjectToJSON and memory leaks?

Errata, please in the above code use the following code in place of TForm1.Button2Click and see what happens:

procedure TForm1.Button1Click(Sender: TObject);
var
  json: RawUTF8;
  i: integer;
  obj: TMyObj;
  res: boolean;
begin
  obj := TMyObj.Create;

  for i := 0 to 1999 do // ObjectToJSON, let's call it 2000 times
    json := ObjectToJSON(obj, [woHumanReadable]);
  for i := 0 to 499 do  // now let's call JSONToObject only 500 times
    JSONToObject(obj, @json[1], res, nil, [j2oIgnoreUnknownProperty]);
  (* the above only leaks 1 time *)

  for i := 0 to 1999 do // now ObjectToJSON and JSONToObject 2000 times in the same cycle
  begin
    json := ObjectToJSON(obj, [woHumanReadable]);
    JSONToObject(obj, @json[1], res, nil, [j2oIgnoreUnknownProperty]);
  end;
  (* the above leaks 2000 times *)

  FreeAndNil(obj);
  MessageDlg('Done. Now close the program to see 2000 memory leaks... one per each iteration of JSONToObject.', mtInformation, [mbOK], 0);
end;

ab

Administrator

From: France

Registered: 2010-06-21

Posts: 14,655

Website

Re: ObjectToJSON and memory leaks?

On top of that, and on a more general scale, I can see that other serializers (like OmniXML) work like a charm with the Assign-based property setter, and don't leak memory. Also, as a side note, OmniXML serializes collections without the need to register them with the serializer (but that might be a side-effect of the greater verbosity of XML versus the data-centric nature of JSON).

In fact, TStrings.Assign() is making a copy of the supplied Value content to the fSomething field.
So using it in a setter is NOT correct.
Just get rid of those getter and setter methods: it will make code slower and more difficult to follow.

In fact, OmniXML is NOT calling your setter.
It is filling the TStrings in-place, using the instance retrieved by the getter.

Check our method function TPropInfo.ClassFromJSON().
If the field has a setter, a new object is created, and assigned to the setter.
This is the right way to do implement it - and it is not what OmniXML does, for instance.

About collections, you can serialize them directly, if you have no new collection instance to create, but already initialized properties without any setter.
The registration is not needed for ObjectToJSON() but when you work on the server side with TCollection parameters of interface-based services: in this case, NO library would be able to create a generic TCollection without knowing the exact collection type.

IMHO you should either fix your setter, or not use any setter.

Anyway, I've added the new j2oSetterExpectsToFreeTempInstance option, to let our function work as you expect.
If you write the following, there won't be any leak any more:

   JSONToObject(obj, @json[1], res, nil, [j2oIgnoreUnknownProperty,j2oSetterExpectsToFreeTempInstance]);

See http://synopse.info/fossil/info/95bd4262df

BBackSoon

Member

Registered: 2014-11-15

Posts: 41

Re: ObjectToJSON and memory leaks?

Thank you. By debugging the code line by line I came to the same conclusion, and I was about to propose the same change to the code, so that both types of "setters" could be supported. But you've been faster than me. You have amazing reaction times! KUDOS! And thanks!

ab

Administrator

From: France

Registered: 2010-06-21

Posts: 14,655

Website

Re: ObjectToJSON and memory leaks?

I should have wait a little bit before committing the enhancement: you would have prepared all the work on your side!

BBackSoon

Member

Registered: 2014-11-15

Posts: 41

Re: ObjectToJSON and memory leaks?

I actually did. Together with a friend of mine we had already made those changes, tested them, and we were getting ready to commit them for your evaluation... but once again you've been faster, so we will just stick to your official changes.
By the way, let me share my appreciation for mORMot once again, it's truly a well-thought-out and well-designed library. My sincere congratulations!