Skip to content

Commit

Permalink
TDosThread without FreeOnTerminate doesn't seem to be threadsafe.
Browse files Browse the repository at this point in the history
  • Loading branch information
Roman Kassebaum committed May 21, 2016
1 parent 20491d3 commit 07da189
Showing 1 changed file with 1 addition and 1 deletion.
2 changes: 1 addition & 1 deletion Source/DosCommand.pas
View file
Original file line number Diff line number Diff line change
Expand Up @@ -569,6 +569,7 @@ constructor TDosThread.Create(AOwner: TDosCommand; ACl, ACurrDir: string; ALines
FMaxTimeAfterLastOutput := AMtalo;
FPriority := Ap;
FTerminateEvent := TEvent.Create(nil, True, False, '');
FreeOnTerminate := False;
end;

destructor TDosThread.Destroy;
Expand Down Expand Up @@ -1163,7 +1164,6 @@ procedure TDosCommand.Stop;
begin
if (FThread <> nil) then
begin
FThread.FreeOnTerminate := False;
FThread.Terminate; // by sirius
FThread.WaitFor; // by sirius2
FreeAndNil(FThread);
Expand Down

2 comments on commit 07da189

@UweRaabe
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TDosCommand läuft nur richtig, wenn FreeOnTerminate = true ist. Andernfalls kann man den Prozess kein zweites Mal starten. Die Ursache liegt in TDosCommand.ThreadTerminated. Dort wird bei FreeOnTerminate nämlich FThread auf nil gesetzt. Intern wird FThread <> nil als Zustand für Running interpretiert.
Der jetzige Code gibt den Thread auch nur im Stop frei. Ohne diesen Aufruf haben wir hier aber ein Speicherleck. Insofern hilft es auch nicht, in ThreadTerminated einfach ein FThread := nil einzusetzen. Damit wird die Thread-Instanz ja nicht freigegeben.

@romankassebaum
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was hältst Du davon, wenn Du das ganze fixed?

Please sign in to comment.