A critical section inside or outside a thread class is better? [Example inside]

Question

I have several threads (Providers), that are used by other threads (Workers) on concurrent basis.

Several threads means severan critical sections according to threads.

Is it impossble to place a critical section variable inside a thread class, or its better to hold separate array for critical section of each thread that needs to be concurrently accessed?

Or there is a better practice?

UPD: a bit explanation.

Process flow is:

1. Worker loads a pool of data to its self
2. In cycle of own pool of data a Worker try to gain access to a Provider
3. Worker gains access to Provider ->
4. Worker locks it ->
5. Worker posts data to Provider from own pool ->
6. Provider start doing a job ->
7. When job is done Provider sends event to Worker ->
8. Worker gets a result ->
9. Worker release lock ->
10. Goto 2 or exit if last data from pool was processed

This actually works fine in a real situation with a very good outcome. But the problem is when I need to kill one of the Providers.

If a critical section inside a Provider class, then its possible to get access violation once a Worker already entered CS and waiting for unlock and a provider is not really terminated yet, but terminated next step.

if not Provider.Terminated then
   Provider.CS.Enter;  //This is a place where AV occurs.
Provider.PostData(Worker.Data);
Provider.StartJob;
WaitForSingleObject(Provider.EventJobIsDone, 30000);
Provider.CS.Leave; //or here 

If I have to place CS outside - how it is better to do this and to test that a provider is terminated?

Several Providers and many Workers, that are created / terminated at any time and should work together. There is no option, that one Worker process all its pool of data while others are waiting. Each Worker do one iteration and then waits while other Workers do the same, then again it do next iteration then again waits while others do the same and so on until all pools are proccessed and Workers are terminated (each on its own time, when pool is finished).

Each iteration of Worker and result it gets influence what Provider would be chosen for that Worker next time.

And again - this works fine in real conditions and no stucks happen. Very fast and quite as it is expected to work.

Example

A rough example.

You have Worker threads, that load a pool of data from DB. There are different DBs, not only one. Each item - is a heavy picture 0.1Mb-10Mb - you never know before. Imagine each Worker has 100-5000 items to be processed - you need to convert it to a lesser picture or do some other process. You never know how many items there would be in any next Worker thread. It could be 10, 1000, 5000, more or less. Whenever a Worker thread is started - there should be no timeout for a new Worker thread to start processing its first item from data pool, because it's a waste of time that costs.

You have 10 Providers, that are connected to 10 machines on the local network. Those machines do all the work you need remotely. (a rough example, please)

You should balance (the real strategy is more complex and depends on the actual data - it's similar to: is it a JPG, TIFF, PNG, BMP or whatever format, but simply please think of balance) traffic among all the Providers correctly. To do this you should count the quantity of processed data and decide each iteration where to put next Worker call - to Provider1,2,3...or 10. Besides you need to return a result immediately to the calling Worker, because a Worker should give an immediate report, write a flag to DB (only Worker knows which DB) that data is processed (!important) and so on.

That's are the conditions. In brief:

• 10-50 Workers at a time with 100-5000 items
• Workers are started and finished on their own
• You can't do a prognosis how many Workers there would be at any time
• Service is working 24/7 and ready to process data
• 10 Providers, each is a connection to a machine on the network
• A new Worker should as soon as possible start processing its data
• A need to balance traffic among Providers on-the-fly
• Amount of processed data thru each Provider affects the decision to choose next Provider for a next data process.
• Worker should always get a result and update data to DB
• Suddenly you are about to terminate a Provider
• Some Workers already waiting to process data with this one Provider

How to tell the Workers to stop waiting and when they are unfreezed - how to avoid Access violation that is pretty sure may occur on the next step in Worker flow (which is presented in the code in above).

Answer 1

Your problem boils down to following scenario:

• A provider thread with a CS inside has to terminate.
• Multiple worker threads could be blocked, waiting for the CS to be released.
• Provider thread is terminated just after the CS is released, causing all pending threads to access a non-existing thread.

---

This is a design problem, never access a thread that could be terminated without notice. You could solve it by some communication. Perhaps let the provider live long enough to tell all other worker threads that it is time to say goodbye. Set a semaphore, answer all pending requests with an end of life message. Make sure the worker threads looks at the semaphore before sending a request.

Answer 2

I have the impression from what you write, that a lot of the system is not under your control. Im still a bit uncertain how much you are allowed to change.

I have implemented a global Controller : TSystemController class which is used to control all the parts.

I understand it is not up to you when a Provider is terminated. Therefore I believe it is only possible to avoid AV by adding a global ProviderJob List/Queue.

The worker will Push jobs and the provider will Pop jobs. A Critical Section is used for consistency during Push / Pop by the many involved threads. I have only used one critical section, since this Queue is responsible for holding all Jobs for the entire system.

A ProviderJob : TProviderJob, holds a ThreadID to the provider thread to avoid reference to a killed/freed provider. Besides the WorkItem a DoneEvent given by the worker is needed, so the Provider can signal when its done to the waiting worker.

If the Provider is suddenly is killed, the worker will just wait for 30 seconds and time out.

TProviderJob = class
  FProviderThreadID : Cardinal;
  FWorkItem : TWorkItem;
  FWorkDoneEvent : TEvent;
public
  property ProviderThreadID : Cardinal read FProviderThreadID;
  property WorkItem : TWorkItem read FWorkItem;
  property WorkDoneEvent : TEvent read FWorkDoneEvent;
end;

The ProviderJob gets pushed by a Worker thread as follows (no direct reference to the Provider object):

ProviderID := Controller.GetProviderThreadID;
Controller.PushWorkItemToProviderJobList( ProviderID, CurrentWorkItem, WorkDoneEvent );

The worker will wait until the Provider is done with the job:

if WaitForSingleObjehect( WorkDoneEvent.Handle, 30000 ) = WAIT_OBJECT_0    then
begin
  if CurrentWorkItem.State = wisProcessedOK then
  begin
    Inc(Controller.FWorkItemsDone);
    Inc(NextWorkItemIdx);
    if NextWorkItemIdx >= WorkItems.Count then
      Terminate;
  end;
end;

The individual Provider is processing jobs as follows:

procedure TProviderThread.Execute;
var
  WorkItem: TWorkItem;
  WorkDoneEvent: TEvent;
begin
  while not Terminated do
  begin
    Controller.PopNextWorkItemFromProviderJobList( self.ThreadID, WorkItem, WorkDoneEvent );
    if (WorkItem<>nil) and (WorkDoneEvent<>nil) then
    begin
      WorkItem.FState := wisStarted;
      Sleep( Round( Random( 5000 )));
      WorkItem.FState := wisProcessedOK;
      WorkDoneEvent.SetEvent;
    end
    else
      Sleep(500);
  end;
end;

Here is an example: Screenshot of the test app

Here is the full solution:

object Form1: TForm1
  Left = 0
  Top = 0
  Caption = 'Form1'
  ClientHeight = 248
  ClientWidth = 477
  Color = clBtnFace
  Font.Charset = DEFAULT_CHARSET
  Font.Color = clWindowText
  Font.Height = -11
  Font.Name = 'Tahoma'
  Font.Style = []
  OldCreateOrder = False
  PixelsPerInch = 96
  TextHeight = 13
  object Memo1: TMemo
    Left = 8
    Top = 8
    Width = 369
    Height = 209
    Lines.Strings = (
      'Memo1')
    TabOrder = 0
  end
  object bStart: TButton
    Left = 383
    Top = 72
    Width = 75
    Height = 25
    Caption = 'Start'
    TabOrder = 1
    OnClick = bStartClick
  end
  object bStop: TButton
    Left = 383
    Top = 103
    Width = 75
    Height = 25
    Caption = 'Stop'
    TabOrder = 2
    OnClick = bStopClick
  end
  object bKillProvider: TButton
    Left = 383
    Top = 134
    Width = 75
    Height = 25
    Caption = 'Kill Provider'
    TabOrder = 3
    OnClick = bKillProviderClick
  end
  object Timer1: TTimer
    OnTimer = Timer1Timer
    Left = 400
    Top = 8
  end
end



unit Unit1;

interface

uses
  Winapi.Windows, Winapi.Messages, System.SysUtils, System.Variants, System.Classes, Vcl.Graphics,
  Vcl.Controls, Vcl.Forms, Vcl.Dialogs, Vcl.StdCtrls, Vcl.ExtCtrls, Math;

type
  TForm1 = class(TForm)
    Timer1: TTimer;
    Memo1: TMemo;
    bStart: TButton;
    bStop: TButton;
    bKillProvider: TButton;
    procedure Timer1Timer(Sender: TObject);
    procedure bKillProviderClick(Sender: TObject);
    procedure bStopClick(Sender: TObject);
    procedure bStartClick(Sender: TObject);
  private
    { Private declarations }
  public
    { Public declarations }
  end;

var
  Form1: TForm1;



implementation

{$R *.dfm}

uses
  System.SyncObjs,
  Generics.Collections;

type
  TWorkItemStateE = ( wisNotStarted, wisStarted, wisProcessedOK );

  TWorkItem = class
  private
    FState : TWorkItemStateE;
    FID : Integer;
  public
    property ID : Integer read FID;
    property State: TWorkItemStateE read FState;
  end;

  TWorkItemList = class(TObjectList<TWorkItem>);

  TWorkerThread = class(TThread)
  protected
    procedure Execute; override;
  end;
  TWorkerThreadList = class(TObjectList<TWorkerThread>);

  TProviderThread = class(TThread)
  protected
    procedure Execute; override;
  end;
  TProviderThreadList = TObjectList<TProviderThread>;

  TProviderJob = class
    FProviderThreadID : Cardinal;
    FWorkItem : TWorkItem;
    FWorkDoneEvent : TEvent;
  public
    property ProviderThreadID : Cardinal read FProviderThreadID;
    property WorkItem : TWorkItem read FWorkItem;
    property WorkDoneEvent : TEvent read FWorkDoneEvent;
  end;
  TProviderJobList = TObjectList<TProviderJob>;

  TSystemContoller = class
  private
    FProviders: TProviderThreadList;
    FWorkers : TWorkerThreadList;
    FGlobalProviderJobList : TProviderJobList;
    FCS_GlobalProviderJobList: TCriticalSection;

    FStarted : Boolean;
    FRemovedProviders: Integer;
    FKilledProviders: Integer;
    FRemovedWorkers: Integer;
    FWorkItemsDone: Integer;
    FStartedDateTime: TDateTime;

    procedure ThreadTerminated(Sender: TObject);
    procedure AddProvider;
    procedure AddWorker;
  public
    constructor Create;
    destructor Destroy; override;

    function GetProvider : TProviderThread;
    function GetProviderThreadID: Cardinal;
    procedure PopNextWorkItemFromProviderJobList(
                         const AProviderThreadID: Cardinal;
                         out NextWorkItem: TWorkItem;
                         out NextWorkDoneEvent: TEvent);
    procedure PushWorkItemToProviderJobList(
                         AProviderThreadID: Cardinal;
                         AWorkItem: TWorkItem;
                         AWorkDoneEvent: TEvent);

    procedure Start;
    procedure Stop;
    procedure KillProvider;

    function GetStatusReport : String;
  end;

var
  Controller : TSystemContoller;




{ TWorkThread }

procedure TWorkerThread.Execute;
procedure LoadWorkItems(  AWorkItems : TWorkItemList );
var
  WorkItemCount: Integer;
  n : Integer;
    WorkItem: TWorkItem;
begin
  // Load work items:
  WorkItemCount := 1+Round(Random(10));
  for n := 1 to WorkItemCount do
  begin
    WorkItem := TWorkItem.Create;
    WorkItem.FID := n;
    AWorkItems.Add(WorkItem);
  end;
end;

var
  WorkItems : TWorkItemList;
  ProviderID: Cardinal;
  NextWorkItemIdx: Integer;
  CurrentWorkItem: TWorkItem;
  WorkDoneEvent : TEvent;
begin
  WorkItems := TWorkItemList.Create;
  WorkDoneEvent := TEvent.Create(nil, False, False, '' );
  try
    // load:
    LoadWorkItems( WorkItems );

    // process work items:
    NextWorkItemIdx := 0;
    while not Terminated do
    begin
      CurrentWorkItem := WorkItems[ NextWorkItemIdx ];
      ProviderID := Controller.GetProviderThreadID;
      Controller.PushWorkItemToProviderJobList( ProviderID, CurrentWorkItem, WorkDoneEvent );

      if WaitForSingleObject( WorkDoneEvent.Handle, 30000 ) = WAIT_OBJECT_0 then
      begin
        if CurrentWorkItem.State = wisProcessedOK then
        begin
          Inc(Controller.FWorkItemsDone);
          Inc(NextWorkItemIdx);
          if NextWorkItemIdx >= WorkItems.Count then
            Terminate;
        end;
      end;
      Sleep(1000);

    end;
  finally
    WorkDoneEvent.Free;
    WorkItems.Free;
  end;
end;


{ TProviderThread }

procedure TProviderThread.Execute;
var
  WorkItem: TWorkItem;
  WorkDoneEvent: TEvent;
begin
  while not Terminated do
  begin
    Controller.PopNextWorkItemFromProviderJobList( self.ThreadID, WorkItem, WorkDoneEvent );
    if (WorkItem<>nil) and (WorkDoneEvent<>nil) then
    begin
      WorkItem.FState := wisStarted;
      Sleep( Round( Random( 5000 )));
      WorkItem.FState := wisProcessedOK;
      WorkDoneEvent.SetEvent;
    end
    else
      Sleep(500);
  end;
end;

{ TSystemContoller }

constructor TSystemContoller.Create;
begin
  inherited;
  FStartedDateTime:= now;

  FCS_GlobalProviderJobList := TCriticalSection.Create;
  FGlobalProviderJobList := TProviderJobList.Create;

  FProviders:= TProviderThreadList.Create;
  FProviders.OwnsObjects := False;

  FWorkers := TWorkerThreadList.Create;
  FWorkers.OwnsObjects := False;
end;

destructor TSystemContoller.Destroy;
begin
  FCS_GlobalProviderJobList.Free;
  FGlobalProviderJobList.Free;
  FWorkers.Free;
  FProviders.Free;
  inherited;
end;

procedure TSystemContoller.Start;
var
  n: Integer;
begin
  if not FStarted then
  begin
    FStarted := True;
    for n := 1 to 5 do
      AddProvider;
    for n := 1 to 10 do
      AddWorker;
  end;
end;

procedure TSystemContoller.Stop;
var
  n: Integer;
begin
  for n := FProviders.Count-1 to 0 do
    FProviders[n].Terminate;

  for n := FWorkers.Count-1 to 0 do
    FWorkers[n].Terminate;

  FStarted := False;
end;

procedure TSystemContoller.KillProvider;
var
  Provider: TProviderThread;
begin
  Provider := GetProvider;
  if Provider<>nil then
  begin
    if not Provider.Terminated then
    begin
      GetProvider.Terminate;
      Inc( FKilledProviders );
    end;
  end;
end;

procedure TSystemContoller.AddProvider;
var
  Provider: TProviderThread;
begin
  Provider := TProviderThread.Create(True);
  Provider.OnTerminate := ThreadTerminated;
  Provider.FreeOnTerminate := True;
  FProviders.Add( Provider );
  Provider.Start;
end;

procedure TSystemContoller.AddWorker;
var
  Worker: TWorkerThread;
begin
  Worker := TWorkerThread.Create(True);
  Worker.OnTerminate := ThreadTerminated;
  Worker.FreeOnTerminate := True;
  FWorkers.Add( Worker );
  Worker.Start;
end;

procedure TSystemContoller.ThreadTerminated(Sender : TObject );
begin
  if Sender is TProviderThread then
  begin
    FProviders.Remove(TProviderThread(Sender));
    Inc(FRemovedProviders);
    if FStarted then
      AddProvider;
  end
  else
  if Sender is TWorkerThread then
  begin
    FWorkers.Remove(TWorkerThread(Sender));
    Inc(FRemovedWorkers);
    if FStarted then
      AddWorker;
  end;
end;

procedure TSystemContoller.PushWorkItemToProviderJobList(
                                     AProviderThreadID: Cardinal;
                                     AWorkItem: TWorkItem;
                                     AWorkDoneEvent: TEvent);
var
  ProviderJob: TProviderJob;
begin
  FCS_GlobalProviderJobList.Enter;
  try
    ProviderJob := TProviderJob.Create;
    ProviderJob.FProviderThreadID := AProviderThreadID;
    ProviderJob.FWorkItem := AWorkItem;
    ProviderJob.FWorkDoneEvent := AWorkDoneEvent;
    FGlobalProviderJobList.Add( ProviderJob );
  finally
    FCS_GlobalProviderJobList.Leave;
  end;
end;

procedure TSystemContoller.PopNextWorkItemFromProviderJobList(
                                   const AProviderThreadID: Cardinal;
                                   out NextWorkItem: TWorkItem;
                                   out NextWorkDoneEvent: TEvent);
var
  n : Integer;
begin
  FCS_GlobalProviderJobList.Enter;
  try
    NextWorkItem := nil;
    NextWorkDoneEvent := nil;
    for n := 0 to FGlobalProviderJobList.Count-1 do
    begin
      if FGlobalProviderJobList[n].ProviderThreadID = AProviderThreadID then
      begin
        NextWorkItem := FGlobalProviderJobList[n].WorkItem;
        NextWorkDoneEvent := FGlobalProviderJobList[n].WorkDoneEvent;
        FGlobalProviderJobList.Delete(n);
        Exit;
      end;
    end;
  finally
    FCS_GlobalProviderJobList.Leave;
  end;
end;

function TSystemContoller.GetProvider: TProviderThread;
var
  ProviderIdx: Integer;
begin
  ProviderIdx := Trunc(Random( FProviders.Count ));
  if InRange(ProviderIdx, 0, FProviders.Count-1 ) then
    Result := FProviders[ ProviderIdx ]
  else
    Result := nil;
end;

function TSystemContoller.GetProviderThreadID: Cardinal;
var
  Provider: TProviderThread;
begin
  Provider := GetProvider;
  if Provider<>nil then
    Result := Provider.ThreadID;
end;

function TSystemContoller.GetStatusReport: String;
const
  cState : array[Boolean] of string = ( 'Stopped', 'Started' );
begin
  Result := 'Start Date and Time: ' + DateTimeToStr(FStartedDateTime) + #13#10+
            'Date and Time: ' + DateTimeToStr(now) + #13#10+
            'System State: ' + cState[FStarted] + #13#10+ #13#10 +
            'Queued Work Items: ' + IntToStr( self.FGlobalProviderJobList.Count )+ #13#10 +
            'Work Items Done: ' + IntToStr(FWorkItemsDone)+ #13#10 + #13#10 +
            'Current Providers: ' + IntToStr( self.FProviders.Count ) + #13#10+
            'Removed Providers: ' + IntToStr( FRemovedProviders ) + #13#10 +
            'Random Provider Kills: ' + IntToStr(FKilledProviders)+ #13#10 + #13#10 +
            'Current Workers: ' + IntToStr( self.FWorkers.Count ) + #13#10 +
            'Removed Workers: ' + IntToStr( FRemovedWorkers );
end;



procedure TForm1.bKillProviderClick(Sender: TObject);
begin
  Controller.KillProvider;
end;

procedure TForm1.bStartClick(Sender: TObject);
begin
  Controller.Start;
end;

procedure TForm1.bStopClick(Sender: TObject);
begin
  Controller.Stop;
end;

procedure TForm1.Timer1Timer(Sender: TObject);
begin
  Memo1.Text :=  Controller.GetStatusReport;

  if Random(100) < 30 then
    Controller.KillProvider;
end;

initialization
  Controller := TSystemContoller.Create;
  Controller.Start;
finalization
  Controller.Stop;
  Controller.Free;
end.