1

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!

2
  • 2
    Check the call stack of the thread that's hanging to determine what's actually causing the hang. Don't just guess.
    – wj32
    Dec 30, 2010 at 2:42
  • Thanks wj32, I didn't know I could check the call stack. Much better the guessing. Dec 31, 2010 at 0:59

8 Answers 8

7

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.

24
  • 1
    @A.Bouchez in fact declaring critical sections as global variables is frequently an excellent design choice Dec 30, 2010 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, 2010 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, 2010 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, 2010 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
    Jan 1, 2011 at 0:37
2

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.

1

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.

1

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.

2
  • 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. Dec 30, 2010 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, 2010 at 18:41
0

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;
9
  • +1 use Try/Finally, this is essential, -1 replace TCriticalSection with TRTLCriticalSection, this is a matter of personal preference. Dec 30, 2010 at 10:38
  • Is acceptable for one thread to acquire a lock and a second thread to release it? Dec 31, 2010 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. Dec 31, 2010 at 8:41
  • @David What about declaring it in the class handling the data? Dec 31, 2010 at 8:43
  • @A.B that's good practice often but the difficulty with such simple examples as this is that it's hard to see where the need for locking arises. Dec 31, 2010 at 8:57
0

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?

5
  • 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." Dec 31, 2010 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? Dec 31, 2010 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, 2011 at 11:04
  • @David. Good point. I was working under the assumption that my code would always run without exceptions. Jan 1, 2011 at 19:44
  • @mghie. Using the lock to protect a variable makes sense. Jan 1, 2011 at 19:50
0

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.

0

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

1
  • Could you please provide a code example? It would make your answer easier to understand. Apr 28, 2013 at 23:38

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.