18

When using TJson.JsonToObject in a multi-thread environment random access violations occur. I was searching a long time for the problem and I could isolate it with the following code

JSON class

type
   TParameter = class
   public
      FName     : string;
      FDataType : string;
      FValue    : string;
   end;

Testfunction:

procedure Test();
var
   myTasks: array of ITask;
   i : integer;
   max : integer;
begin

  max := 50;
  SetLength(myTasks, max);
  for i := 0 to max  -1  do begin
     myTasks[i] := TTask.Create(procedure ()
       var
          json : string;
          p : TParameter;
       begin
          json := '{"name":"NameOfParam","dataType":"TypeOfParam","value":"ValueOfParam"}';
          p := TJson.JsonToObject<TParameter>(json);
          p.Free;
       end);
     myTasks[i].Start;
  end;

  TTask.WaitForAll(myTasks);
  ShowMessage('all done!');
end;

It's only a code snippet based of a much more complex source. As long I use this code in a single thread everything works without a problem. I'm wondering if there is anything wrong with the code.

Cœur
  • 37,241
  • 25
  • 195
  • 267
deterministicFail
  • 1,271
  • 9
  • 28
  • Which JSON library? Is it known to be thread safe? Why with a lock around the call to `JsonToObject`. If the code runs well with the lock, and fails without then that would point to `JsonToObject` not being thread safe. – David Heffernan Feb 05 '15 at 10:19
  • I am unable to reproduce this. Just setting up a console application and putting your `TParameter` in a separate unit seems to work fine, no exceptions.Addings a `WriteLn( TThread.CurrentThread.ThreadID );` to the `Proc` of your `TTask` also shows that de-serialization occurs from different threads. – Günther the Beautiful Feb 05 '15 at 10:28
  • @DavidHeffernan the Library is from embarcadero REST.Json – deterministicFail Feb 05 '15 at 10:44
  • @GünthertheBeautiful i tried it with a console application but had the same problem, do you have installed any xe7 patches? – deterministicFail Feb 05 '15 at 10:44
  • @deterministicFail I have to revoke my hasty statement. I'm running XE7 without Update 1. When running the tasks in a `TThreadPool` with the minimum number of worker threads set to greater than one, it fails sooner or later. The higher the number, the sooner it crashes. – Günther the Beautiful Feb 05 '15 at 10:51
  • 3
    I think that the problem is in the RTTI. It's just not thread-safe. For instance: http://stackoverflow.com/questions/27368556/trtticontext-multi-thread-issue I expect that there are more problems along these lines – David Heffernan Feb 05 '15 at 11:45
  • 5
    Yes, I'm confident that there's a thread safety issue. If I serialize the execution of `TJSONUnMarshal.CreateObject` then there are no errors. I think you need to submit a bug report. You've got a beautiful reproduction. – David Heffernan Feb 05 '15 at 12:02

1 Answers1

15

The method TJSONUnMarshal.ObjectInstance in REST.JsonReflect.pas has a severe bug:

It calls FreeAndNil on a TRttiType instance. This should never be done because all TRtti*** instances are managed by the TRttiContext.

After I removed the FreeAndNil call I could not reproduce the access violation anymore.

Reported as: https://quality.embarcadero.com/browse/RSP-10035

P.S. I also think that https://quality.embarcadero.com/browse/RSP-9815 will affect your code.

Stefan Glienke
  • 20,860
  • 2
  • 48
  • 102