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

Question

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)?

Answer 1

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.