[fl4shc0d3r]

I have this code below that send email based in queue threads but if not pause (eg: Sleep(1000)) by a certain time, two threads try access the same item of queue simultaneous.

Someone know how solve this trouble?

Code: Select all

uses
  System.Types, Generics.Collections, IdMessage;

type
  TThreadItem = class;
  TThreadList = TObjectList;
  TMessageItem = TIdMessage;
  TMessageQueue = TThreadedQueue;

  TThreadPool = class
  private
    FQueue: TMessageQueue;
    FThreads: TThreadList;
  public
    constructor Create(Count: Integer);
    destructor Destroy; override;
    procedure Shutdown;
    property Queue: TMessageQueue read FQueue;
  end;

  TThreadItem = class(TThread)
  private
    FQueue: TMessageQueue;
  protected
    procedure Execute; override;
  public
    constructor Create(Queue: TMessageQueue); reintroduce;
  end;

implementation

{ TThreadPool }

constructor TThreadPool.Create(Count: Integer);
var
  I: Integer;
  Thread: TThreadItem;
begin
  inherited Create;
  { this will create thread queue that will wait for push and pop of its items INFINITE
    time; that's useful for thread sleeping }
  FQueue := TMessageQueue.Create;
  FThreads := TThreadList.Create;

  for I := 0 to Count-1 do
  begin
    Thread := TThreadItem.Create(FQueue);
    FThreads.Add(Thread);
  end;
end;

destructor TThreadPool.Destroy;
begin
  Shutdown;
  FThreads.Free;
  FQueue.Free;
  inherited;
end;

procedure TThreadPool.Shutdown;
var
  Thread: TThreadItem;
  Message: TMessageItem;
begin
  { signal threads for termination }
  for Thread in FThreads do
    Thread.Terminate;
  { shutdown the queue to "unlock" sleeping threads }
  FQueue.DoShutDown;
  { free all the unprocessed enqueued message items }
  Message := FQueue.PopItem;
  while Assigned(Message) do
  begin
    Message.Free;
    Message := FQueue.PopItem;
  end;
end;

{ TThreadItem }

constructor TThreadItem.Create(Queue: TMessageQueue);
begin
  inherited Create;
  FQueue := Queue;
end;

procedure TThreadItem.Execute;
var
  Message: TMessageItem;
begin
  { <- create and setup Indy sending object here }
  try
    while not Terminated do
      { here we'll wait for INFINITE time for an item or until queue is shutted down;
        you should consider checking for error state as well }
      if FQueue.PopItem(Message) = wrSignaled then
      try
        { <- send the Message through the Indy sending object here }
      finally
        Message.Free;
      end;
  finally
    { <- destroy Indy sending object here }
  end;
end;

////////////////////////////////////////////////////////////////////////////////////////////////////////////

{ Possible usage: }

Pool := TThreadPool.Create(2); { <- create 2 threads }
for I := 0 to 99 do
begin
  Message := TMessageItem.Create(nil);
  Message.Subject := 'Message subject';
  ...
  Pool.Queue.PushItem(Message);
end;

[Vrtule]

Sleep/wakeup SHOULD NEVER be used for synchronizing accesses to a shared variable. The reason is that these operations are not atomic and make the program dependent on the processor performance, mood of the thread scheduler etc. (and one of the basic rules of the critical section says that you should never ever do this).

Instead, use mutexes (CreateMutex, WaitForXxxObject, ReleaseMutex, CloseHandle) or critical sections (InitializeCriticalSection, EnterCriticalSection, LeaveCriticalSection, DeleteCriticalSection). The trick is that either a mutex, or a critical section, can be owned at most by one thread at any time moment. And that is exactly what you need, since at most one of your threads should send the message (precisely, extract it from the queu) at once.

IIRC, Delphi has an unit called SyncObj which contains a wrapper around Windows criticalsections, so you do not need to use Windows API directly.

[Brock]

Vrtule is right however if you want to use WinAPI directly it's only a few lines of code and you don't need classes or OO for it.

Code: Select all

var
   CritSec: RTL_CRITICAL_SECTION;


procedure EnterLock;
begin
    EnterCriticalSection(CritSec);
end;


procedure LeaveLock;
begin
    LeaveCriticalSection(CritSec);
end;


procedure AccessMutualResource;
begin
       EnterLock;
   try
   // Access mutual resource here
   finally
       LeaveLock;
   end;
end;


initialization
     InitializeCriticalSection(CritSec);


finalization
     DeleteCriticalSection(CritSec);

[fl4shc0d3r]

Vrtule is right however if you want to use WinAPI directly it's only a few lines of code and you don't need classes or OO for it.

Code: Select all

var
   CritSec: RTL_CRITICAL_SECTION;


procedure EnterLock;
begin
    EnterCriticalSection(CritSec);
end;


procedure LeaveLock;
begin
    LeaveCriticalSection(CritSec);
end;


procedure AccessMutualResource;
begin
       EnterLock;
   try
   // Access mutual resource here
   finally
       LeaveLock;
   end;
end;


initialization
     InitializeCriticalSection(CritSec);


finalization
     DeleteCriticalSection(CritSec);

Ok, solved.

[EP_X0FF]

Ok, solved.

Copy-pasted you mean. Next time do your outstanding quality topics in appreciate forum for newbie questions.