Passing string-vars to Thread is unsafe?

Question

If I run this code, the outcome is unexpected as the strings in the thread dont have the correct values:

procedure TMain.FollowBose(Speakername: string; NewState: Boolean);
{..}
procedure TMain.ProcessRoomMotionResponse(Rooms: TJSONArray; Response: TJSONObject);
var
  ParamName1: string;
  ParamName2: string;
  Room: tJSONValue;
  RoomData: TJSONObject;
  Roomname: string;
  Motion: Boolean;
begin
  if Assigned(Rooms) then begin
    ParamName1 := 'Room';
    ParamName2 := 'Motion';
    for Room in Rooms do begin
      RoomData := Room as TJSONObject;
      if Assigned(RoomData.Values[ParamName1]) and Assigned(RoomData.Values[ParamName2]) then begin
        Roomname := RoomData.Values[ParamName1].Value;
        Motion := RoomData.Values[ParamName2].AsType<Boolean>;
        LogWrite('Launching sub for ' + Roomname + '-FollowUp.', Debug);
        TThread.CreateAnonymousThread(
          procedure
          begin
            FollowUp(Roomname, Motion);
          end).Start;
      end;
    end;
  end;
end;

Is this to be expected because of Strings being special?

Answer 1

This has nothing to do with strings specifically, but with how anonymous procedures capture variables. This is documented behavior:

Anonymous Methods in Delphi > Anonymous Methods Variable Binding

In particular, per the section on "Variable Binding Mechanism":

If an anonymous method refers to an outer local variable in its body, that variable is "captured". Capturing means extending the lifetime of the variable, so that it lives as long as the anonymous method value, rather than dying with its declaring routine. Note that variable capture captures variables--not values. If a variable's value changes after being captured by constructing an anonymous method, the value of the variable the anonymous method captured changes too, because they are the same variable with the same storage. Captured variables are stored on the heap, not the stack.

You are sharing the same Roomname and Motion local variables with all of the threads, instead of giving each thread its own copy of the variables. While the loop is running, you are changing the values of the variables that all of the threads are sharing.

Try this instead:

procedure TMain.ProcessRoomMotionResponse(Rooms: TJSONArray; Response: TJSONObject);
var
  ParamName1: string;
  ParamName2: string;
  Room: tJSONValue;
  RoomData: TJSONObject;
  Roomname: string;
  Motion: Boolean;

  procedure DoFollowUp(ARoomName: string; AMotion: Boolean);
  begin
    LogWrite('Launching sub for ' + ARoomName + '-FollowUp.', Debug);
    TThread.CreateAnonymousThread(
      procedure
      begin
        FollowUp(ARoomName, AMotion);
      end).Start;
  end;

begin
  if Assigned(Rooms) then begin
    ParamName1 := 'Room';
    ParamName2 := 'Motion';
    for Room in Rooms do begin
      RoomData := Room as TJSONObject;
      if Assigned(RoomData.Values[ParamName1]) and Assigned(RoomData.Values[ParamName2]) then begin
        Roomname := RoomData.Values[ParamName1].Value;
        Motion := RoomData.Values[ParamName2].AsType<Boolean>;
        DoFollowUp(Roomname, Motion);
      end;
    end;
  end;
end;

This way, each thread is capturing its own copy of the input parameters of DoFollowUp(), instead of capturing the local variables of ProcessRoomMotionResponse().