Why does my multi-threaded app sometimes hang when it closes?

StackOverflow https://stackoverflow.com/questions/4559234

Question

I'm using several critical sections in my application. The critical sections prevent large data blobs from being modified and accessed simultaneously by different threads.

AFAIK it's all working correctly except sometimes the application hangs when exiting. I'm wondering if this is related to my use of critical sections.

Is there a correct way to free TCriticalSection objects in a destructor?

Thanks for all the answers. I'm looking over my code again with this new information in mind. Cheers!

Was it helpful?

Solution

As Rob says, the only requirement is that you ensure that the critical section is not currently owned by any thread. Not even the thread about to destroy it. So there is no pattern to follow for correctly destroying a TCriticalSection, as such. Only a required behaviour that your application must take steps to ensure occurs.

If your application is locking then I doubt it is the free'ing of any critical section that is responsible. As MSDN says (in the link that Rob posted), the DeleteCriticalSection() (which is ultimately what free'ing a TCriticalSection calls) does not block any threads.

If you were free'ing a critical section that other threads were still trying to access you would get access violations and other unexpected behaviours, not deadlocks, as this little code sample should help you demonstrate:

implementation

uses
  syncobjs;


  type
    tworker = class(tthread)
    protected
      procedure Execute; override;
    end;


  var
    cs: TCriticalSection;
    worker: Tworker;


procedure TForm2.FormCreate(Sender: TObject);
begin
  cs := TCriticalSection.Create;

  worker := tworker.Create(true);
  worker.FreeOnTerminate := TRUE;
  worker.Start;

  sleep(5000);

  cs.Enter;

  showmessage('will AV before you see this');
end;

{ tworker }

procedure tworker.Execute;
begin
  inherited;
  cs.Free;
end;

Add to the implementation unit of a form, correcting the "TForm2" reference for the FormCreate() event handler as required.

In FormCreate() this creates a critical section then launches a thread whose sole purpose is to free that section. We introduce a Sleep() delay to give the thread time to initialise and execute, then we try to enter the critical section ourselves.

We can't of course because it has been free'd. But our code doesn't hang - it is not deadlocked trying to access a resource that is owned by something else, it simply blows up because, well, we're trying to access a resource that no longer exists.

You could be even more sure of creating an AV in this scenario by NIL'ing the critical section reference when it is free'd.

Now, try changing the FormCreate() code to this:

  cs := TCriticalSection.Create;

  worker := tworker.Create(true);
  worker.FreeOnTerminate := TRUE;

  cs.Enter;
  worker.Start;

  sleep(5000);

  cs.Leave;

  showmessage('appearances can be deceptive');

This changes things... now the main thread will take ownership of the critical section - the worker thread will now free the critical section while it is still owned by the main thread.

In this case however, the call to cs.Leave does not necessarily cause an access violation. All that occurs in this scenario (afaict) is that the owning thread is allowed to "leave" the section as it would expect to (it doesn't of course, because the section has gone, but it seems to the thread that it has left the section it previously entered) ...

... in more complex scenarios an access violation or other error is possibly likely, as the memory previously used for the critical section object may be re-allocated to some other object by the time you call it's Leave() method, resulting in some call to some other unknown object or access to invalid memory etc.

Again, changing the worker.Execute() so that it NIL's the critical section ref after free'ing it would ensure an access violation on the attempt to call cs.Leave(), since Leave() calls Release() and Release() is virtual - calling a virtual method with a NIL reference is guaranteed to AV (ditto for Enter() which calls the virtual Acquire() method).

In any event:

Worst case: an exception or weird behaviour

"Best" case: the owning thread appears to believe it has "left" the section as normal.

In neither case is a deadlock or a hang going to occur simply as the result of when a critical section is free'd in one thread in relation to when other threads then try to enter or leave that critical section.

All of which is a round-a-bout way of saying that it sounds like you have a more fundamental race condition in your threading code not directly related to the free'ing of your critical sections.

In any event, I hope my little bit of investigative work might set you down the right path.

OTHER TIPS

Just make sure nothing still owns the critical section. Otherwise, MSDN explains, "the state of the threads waiting for ownership of the deleted critical section is undefined." Other than that, call Free on it like you do with all other objects.

AFAIK it's all working correctly except sometimes the application hangs when exiting. I'm wondering if this is related to my use of critical sections.

Yes it is. But the problem is likely not in the destruction. You probably have a deadlock.

Deadlocks are when two threads wait on two exclusive resources, each wanting both of them and each owning only one:

//Thread1:
FooLock.Enter;
BarLock.Enter;

//Thread2:
BarLock.Enter;
FooLock.Enter;

The way to fight these is to order your locks. If some thread wants two of them, it has to enter them only in specific order:

//Thread1:
FooLock.Enter;
BarLock.Enter;

//Thread2:
FooLock.Enter;
BarLock.Enter;

This way deadlock will not occur.

Many things can trigger deadlock, not only TWO critical sections. For instance, you might have used SendMessage (synchronous message dispatch) or Delphi's Synchronize AND one critical section:

//Thread1:
OnPaint:
  FooLock.Enter;
  FooLock.Leave;

//Thread2:
FooLock.Enter;
Synchronize(SomeProc);
FooLock.Leave;

Synchronize and SendMessage send messages to Thread1. To dispatch those messages, Thread1 needs to finish whatever work it's doing. For instance, OnPaint handler.

But to finish painting, it needs FooLock, which is taken by Thread2 which waits for Thread1 to finish painting. Deadlock.

The way to solve this is either to never use Synchronize and SendMessage (the best way), or at least to use them outside of any locks.

Is there a correct way to free TCriticalSection objects in a destructor?

It does not matter where you are freeing TCriticalSection, in a destructor or not.

But before freeing TCriticalSection, you must ensure that all the threads that could have used it, are stopped or are in a state where they cannot possibly try to enter this section anymore.

For example, if your thread enters this section while dispatching a network message, you have to ensure network is disconnected and all the pending messages are processed.

Failing to do that will in most cases trigger access violations, sometimes nothing (if you're lucky), and rarely deadlocks.

There are no magical in using TCriticalSection as well as in critical sections themselves. Try to replace TCriticalSection objects with plain API calls:

uses
  Windows, ...

var
  CS: TRTLCriticalSection;

...

EnterCriticalSection(CS);
....
here goes your code that you have to protect from access by multiple threads simultaneously
...
LeaveCriticalSection(FCS);
...

initialization
  InitializeCriticalSection(CS);

finalization
  DeleteCriticalSection(CS);

Switching to API will not harm clarity of your code, but, perhaps, help to reveal hidden bugs.

You NEED to protect all critical sections using a try..finally block.

Use TRTLCriticalSection instead of a TCriticalSection class. It's cross-platform, and TCriticalSection is only an unnecessary wrapper around it.

If any exception occurs during the data process, then the critial section is not left, and another thread may block.

If you want fast response, you can also use TryEnterCriticalSection for some User Interface process or such.

Here are some good practice rules:

  1. make your TRTLCriticalSection a property of a Class;
  2. call InitializeCriticalSection in the class constructor, then DeleteCriticalSection in the class destructor;
  3. use EnterCriticalSection()... try... do something... finally LeaveCriticalSection(); end;

Here is some code sample:

type
  TDataClass = class
  protected
    fLock: TRTLCriticalSection;
  public
    constructor Create;
    destructor Destroy; override;
    procedure SomeDataProcess;
  end;

constructor TDataClass.Create;
begin
  inherited;
  InitializeCriticalSection(fLock);
end;

destructor TDataClass.Destroy;
begin
  DeleteCriticalSection(fLock);
  inherited;
end;

procedure TDataClass.SomeDataProcess;
begin
  EnterCriticalSection(fLock);
  try
    // some data process
  finally
    LeaveCriticalSection(fLock);
  end;
end;

If the only explicit synchronisation code in your app is through critical sections then it shouldn't be too difficult to track this down.

You indicate that you have only seen the deadlock on termination. Of course this doesn't mean that it cannot happen during normal operation of your app, but my guess (and we have to guess without more information) is that it is an important clue.

I would hypothesise that the error may be related to the way in which threads are forcibly terminated. A deadlock such as you describe would happen if a thread terminated whilst still holding the lock, but then another thread attempted to acquire the lock before it had a chance to terminate.

A very simple thing to do which may fix the problem immediately is to ensure, as others have correctly said, that all uses of the lock are protected by Try/Finally. This really is a critical point to make.

There are two main patterns for resource lifetime management in Delphi, as follows:

lock.Acquire;
Try
  DoSomething();
Finally
  lock.Release;
End;

The other main pattern is pairing acquisition/release in Create/Destroy, but that is far less common in the case of locks.

Assuming that your usage pattern for the locks is as I suspect (i.e. acquireand release inside the same method), can you confirm that all uses are protected by Try/Finally?

If your application only hangs/ deadlocks on exit please check the onterminate event for all threads. If the main thread signals for the other threads to terminate and then waits for them before freeing them. It is important not to make any synchronised calls in the on terminate event. This can cause a dead lock as the main thread waits for the worker thread to terminate. But the synchronise call is waiting on the main thread.

Don't delete critical sections at object's destructor. Sometimes will cause your application to crash.

Use a seperate method which deletes the critical section.

procedure someobject.deleteCritical();
begin
DeleteCriticalSection(criticalSection);
end;

destructor someobject.destroy();
begin
// Do your release tasks here
end;

1) You call delete critical section
2) After you release(free) the object

Licensed under: CC-BY-SA with attribution
Not affiliated with StackOverflow
scroll top