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

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.

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.

1 @A. Bouchez in fact declaring critical sections as global variables is frequently an excellent design choice – David Heffernan Dec 30 '10 at 10:16 1 @David: No, it almost never is, because a CS should have close relation to the data it protects, which will probably not be global either. Locking should also be as fine-grained as possible, and strict control over the locking hierarchy is necessary to prevent deadlocks.

All of this seems to point to CS being (in general) more local. – mghie Dec 30 '10 at 10:26 1 @David: I agree that the downvote was uncalled for, however I stand by my point. I don't buy your argument that "declaring critical sections as global variables is frequently an excellent design choice" - it may be necessary in rare cases (a global logging framework for example isn't one of those), but it's a necessary evil then, not a good choice.

– mghie Dec 30 '10 at 10:42 1 @ All - there is no such thing as a "global variable" in Delphi. The highest visibility that is possible is unit-interface. This is close enough to what other languages call "global" that it is often confused as such.

The lengths that people go to to obfuscate the fact that sometimes data has to have such high visibility, allowing themselves to sleep at night pretending that they aren't using a "global" variable is ridiculous... pity the poor people who have to maintain the code and de-obfuscate the spaghetti of singleton management code only to realise that "Aaaahhhh, it's a global! " – Deltics Dec 30 '10 at 18:40 1 @A B - Prohibiting globals is the bad design choice (for a language) because it imposes limits on application designers: witness the syntactic verbosity that this design choice then imposes on such languages, to declare simple things like symbolic constants, requiring an arbitrary "container" class. Globals are neither good nor bad, per se.

Only specific uses of them can be labelled as such. Dogmatically dismissing all uses of them as bad is simply lazy, side-stepping the business of engaging ones brain in order to make a proper assessment.... – Deltics Dec 30 '107 at 0:37.

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.

1 it's hard to see how this could help. The code will be just as clear either way. However you do it the enter/leave pair must be protected by a Try Finally block.

– David Heffernan Dec 30 '10 at 10:18 I echo that David, plus eliminating the TCriticalSection removes one additional debugging tool - the ability to NIL the reference to highlight uses of the reference after it has been free'd. – Deltics Dec 30 '10 at 18:41.

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: make your TRTLCriticalSection a property of a Class; call InitializeCriticalSection in the class constructor, then DeleteCriticalSection in the class destructor; 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.

1 use Try/Finally, this is essential, -1 replace TCriticalSection with TRTLCriticalSection, this is a matter of personal preference. – David Heffernan Dec 30 '10 at 10:38 Is acceptable for one thread to acquire a lock and a second thread to release it? – Shannon Dec 31 '10 at 1:07 @Shannon Check the official documentation of LeaveCriticalSection: "If a thread calls LeaveCriticalSection when it does not have ownership of the specified critical section object, an error occurs that may cause another thread using EnterCriticalSection to wait indefinitely."

So it's definitively NOT acceptable. – Arnaud Bouchez Dec 31 '10 at 8:41 @David What about declaring it in the class handling the data? – Arnaud Bouchez Dec 31 '10 at 8:43 @shannon NO NO NO it is totally unacceptable!

– David Heffernan Dec 31 '10 at 8:54.

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?

Thanks for the answer David. – Shannon Dec 31 '10 at 0:41 I don't see how I can use Try/Finally blocks in my situation. I'm using a thread to load large files to avoid blocking other parts of the application.

The lock says "This file is being loaded now, do not interrupt or use the data. " – Shannon Dec 31 '10 at 0:50 @shannon I don't see how you draw that conclusion. How can you be sure that your acquire will pair with a release if you don't use Try/Finally?

– David Heffernan Dec 31 '10 at 0:56 @Shannon: See my other comment. A lock says no such thing, if you want that look into events and semaphores, or use the lock only to protect the variable that says "file is being loaded". – mghie Jan 1 at 11:04 @David.

Good point. I was working under the assumption that my code would always run without exceptions. – Shannon Jan 1 at 19:44.

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.

I cant really gove you an answer,but what I can give you is a way to a solution, that is you have to find the anglde that you relate to or peaks your interest. A good paper is one that people get drawn into because it reaches them ln some way.As for me WW11 to me, I think of the holocaust and the effect it had on the survivors, their families and those who stood by and did nothing until it was too late.