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

Wow!!! JVCL on D5! i though it was plugged off after i quit it and stopped maintaining D5 compatibility. Such a nostalgic feeling... — Arioch 'The, Jul 19 '13 at 14:51
@Arioch'The Don't be too nostalgic; it's JVCL 3.x from 2009. And strictly speaking it's Richard Marquand's original HidController class from 2005; which i helped a little bit with. The version adopted by JVCL underwent a huge *"jcl-ifying"*; but there's no real differences; but technically i'm using Richard's version; so i can fix the *use-after-free* crash that FastMM catches. — Ian Boyd, Jul 19 '13 at 15:39

David Heffernan · Accepted Answer · 2013-07-19T14:27:10.307

12

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.

edited Jul 19 '13 at 14:27

answered Jul 19 '13 at 14:21

David Heffernan

601,492
42
1,072
1,490

That explains it. +1 for the history lesson! – Ian Boyd Jul 19 '13 at 15:33
The way I always do it is instantiate/destroy directly in the thread execution. That must be done anyway if you need to work with things like COM (such as ADO). So, in reality, whenever I write a thread, I never implement any creation or destruction or anything for that matter in create/destroy. (+1) – Jerry Dodge Jul 19 '13 at 17:14
@Jerry that's fine until you need to communicate between creator and thread – David Heffernan Jul 19 '13 at 20:04
"*One way to work around the issue in Delphi 5 is to call the inherited constructor last*" - another way is to replicate what `TThread` does in D6+. Call `inherited` with `CreateSuspended=True` in your constructor (then order does not matter) and then override `AfterConstruction()` to call `Resume()`. Rather than requiring the calling code that constructs the thread object to call `Resume()` manually. – Remy Lebeau Sep 08 '17 at 15:43

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

1 Answers1

Linked