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

1

9

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

Tetralogy answered 19/7, 2013 at 14:17 Comment(2)
Wow!!! JVCL on D5! i though it was plugged off after i quit it and stopped maintaining D5 compatibility. Such a nostalgic feeling...Eduardo
@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.Tetralogy
F
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.

Feeble answered 19/7, 2013 at 14:21 Comment(4)
That explains it. +1 for the history lesson!Tetralogy
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)Inquiline
@Jerry that's fine until you need to communicate between creator and threadFeeble
"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.Ilse

© 2022 - 2024 — McMap. All rights reserved.