Delphi: Should a thread ever be created "not suspended"?

delphi, delphi-5, jvcl, multithreading

Solution

This is a basic design flaw with the Delphi 5 implementation of `TThread`. The underlying Windows thread is started in the constructor of `TThread`. Which leads to the race that you describe.

In the Delphi 6 version of the RTL, the thread start mechanism was changed. From Delphi 6 onwards, the thread is started in `TThread.AfterConstruction`. And that runs after the constructor completes. That would render your code race free.

In Delphi 6 and later, the underlying Windows thread is created in the `TThread` constructor, but is created suspended using the `CREATE_SUSPENDED` flag. Then in `AfterConstruction`, so long as `TThread.FCreateSuspended` is `False`, the thread is resumed.

One way to work around the issue in Delphi 5 is to call the inherited constructor last. Like this:

constructor TJvHidDeviceReadThread.CtlCreate(const Dev: TJvHidDevice);
begin
  Device := Dev;
  NumBytesRead := 0;
  SetLength(Report, Dev.Caps.InputReportByteLength);
  inherited Create(False);
end;

Rather ugly I know.

So, your approach of creating the thread suspended and resuming once the constructor has completed is probably better. That approach mirrors how the RTL solves the problem in Delphi 6 and up.

Problem

I've been trying to track down a memory leak in Jedi VCL's `JvHidControllerClass.pas`, which i came across this change in the source history: Older revision: ``` constructor TJvHidDeviceReadThread.CtlCreate(const Dev: TJvHidDevice); begin inherited Create(True); Device := Dev; NumBytesRead := 0; SetLength(Report, Dev.Caps.InputReportByteLength); end; ``` Current revision: ``` constructor TJvHidDeviceReadThread.CtlCreate(const Dev: TJvHidDevice); begin inherited Create(False); Device := Dev; NumBytesRead := 0; SetLength(Report, Dev.Caps.InputReportByteLength); end; ``` From experience, i've discovered that if you create a thread not suspended: ``` inherited Create(False); ``` then the thread immediately starts running. In this case it will attempt to access an object that has not been initialized yet: ``` procedure TJvHidDeviceReadThread.Execute; begin while not Terminated do begin FillChar(Report[0], Device.Caps.InputReportByteLength, #0); if Device.ReadFileEx(Report[0], Device.Caps.InputReportByteLength, @DummyReadCompletion) then ``` right away trying to fill `Report`, and access the object `Device`. Problem is that they haven't been initialized yet; those are the next lines after the thread has started: ``` Device := Dev; NumBytesRead := 0; SetLength(Report, Dev.Caps.InputReportByteLength); ``` i realize this is a race condition; and the chances of a user experiencing a crash in production are pretty low, so it's probably harmless to leave the race-crash. But am i way off? Am i missing something? Does calling: ``` BeginThread(nil, 0, @ThreadProc, Pointer(Self), Flags, FThreadID); ``` not start a thread off and running right away? Is this really a race condition regression that was (intentionally) added to JVCL? Is there some secret about ``` CreateSuspended(False); ``` that makes it the correct code over: ``` CreateSuspended(True); ... FDataThread.Resume; ``` ? After having been burned by mistakenly calling ``` TMyThread.Create(False) ``` i've filed it in my brain as never correct. Is there any valid use for letting a thread start right away (when you have to initialize values)?

Original source