A stricter TCriticalSection

[MarkMLl]

I've got a fairly large chunk of code here which was originally non-threaded Javascript, which I've converted to FPC with threads. I've got a few residual problems, which I'm fairly sure are due to one or more threads calling code reentrantly.

I've "tightened up" TCriticalSection to also- hopefully- catch cases of same-thread reentry: I don't really care if reentry causes a thread to block since once I know that something's jammed I can find it fairly easily with a debugger.

I'm the first to admit that I'm not the strongest CompSci guy around here. Please could somebody eyeball the code below, and let me know if there's something obvious that I'm overlooking (i.e. abuse of the increment/decrement functions).

Code: Pascal  [Select][+][-]

1. type
2.   TCriticalSectionStrictness= (esRelaxed, esStrictBlocking, esStrictFatal);
3.  
4. const
5.   esStrictnessDefault= esStrictFatal;       (* See enumeration immediately above    *)
6.  
7. type
8.   TStrictCriticalSection=
9.   class(TCriticalSection)
10.   strict private
11.     fStrictness: TCriticalSectionStrictness;
12.     fEntered: Int64;
13.   public
14.     constructor Create(strictness: TCriticalSectionStrictness= esStrictnessDefault);
15.     procedure Enter(const caller: ansistring= '');
16.     procedure Leave(const caller: ansistring= '');
17. end;
18.  
19. constructor TStrictCriticalSection.Create(strictness: TCriticalSectionStrictness= esStrictnessDefault);
20.  
21. begin
22.   inherited Create;
23.   fStrictness := strictness;
24.   fEntered := 0
25. end { TStrictCriticalSection.Create } ;
26.  
27. procedure TStrictCriticalSection.Enter(const caller: ansistring= '');
28.  
29. begin
30.   inherited Enter;
31.   case fStrictness of
32.     esStrictBlocking: while InterlockedIncrement64(fEntered) > 1 do begin
33.                         InterlockedDecrement64(fEntered);
34.                         Sleep(0)
35.                       end;
36.     esStrictFatal:    if InterlockedIncrement64(fEntered) > 1 then begin
37.                         InterlockedDecrement64(fEntered);
38.                         raise Exception.Create('Enter reentered' + caller)
39.                       end
40.   otherwise
41.     InterlockedIncrement64(fEntered)
42.   end
43. end { TStrictCriticalSection.Enter } ;
44.  
45. procedure TStrictCriticalSection.Leave(const caller: ansistring= '');
46.  
47. begin
48.   case fStrictness of
49.     esStrictFatal: if InterlockedDecrement64(fEntered) < 0 then
50.                      raise Exception.Create('Leave reexited' + caller)
51.   otherwise
52.     InterlockedDecrement64(fEntered)
53.   end;
54.   inherited Leave
55. end { TStrictCriticalSection.Leave } ;
56.  

I've omitted Acquire/Release/TryEnter from the above example, since they essentially duplicate the procedures shown.

Criticism appreciated :-)

MarkMLl

[Martin_fr]

Am I missing something? But after the "inherited Enter" only one thread can reach at a time? So how can the counter ever be anything but zero (being increased to 1) ?
Well, it can if the same thread already entered (never mind via which calling function). But if that is the case that thread will be waiting forever for the counter to go down (since if it is in this loop, it wont go to "leave" ever). So only the exception makes sense here.

Besides this, if you want to test your critical section usage, and if you can run on Linux => valgrind has 2 thread checkers.
The will report a lot of false positives. But they will also tell you if multiple sections are entered in different order (dead lock potential).

[MarkMLl]

Am I missing something? But after the "inherited Enter" only one thread can reach at a time? So how can the counter ever be anything but zero (being increased to 1) ?
Well, it can if the same thread already entered (never mind via which calling function).

Precisely :-)

But if that is the case that thread will be waiting forever for the counter to go down (since if it is in this loop, it wont go to "leave" ever). So only the exception makes sense here.

Don't care. At present some code is being called reentrantly which shouldn't... as things stand my modified critical section has pointed me at a strong suspect but I'm interested in case I've coded it wrong: bearing in mind the rigour brought to this by Dijkstra and others.

MarkMLl

[Thaddy]

@Mark

I just not like otherwise....
My brain compiler can not parse it.

(unless I am freezing, then I remember ISO)

[MarkMLl]

I just not like otherwise....
My brain compiler can not parse it.

Thanks for that Thaddy, if it's your only kvetch then I'm content (I agree that  otherwise  is jarring, but I have memories of a hard-to-find bug caused by using  else  in this context).

I've now got my code to deliver a hard error, which is a decided improvement. I've added a few lines to save "caller" as "holder" on successful entry and can see /what's/ happening, now all I have to do is get a grip on /why/ it's happening and then on /how/ to avoid it.

This isn't the first time I've had to contend with TCriticalSection letting the same thread through, hence another recent comment of mine relating to A.PM and Synchronize(shim).

MarkMLl

[Thaddy]

i noticed a small issue:
AFAIK sleep(0) in the meaning of "relinquish time slice" works only on windows, so your new code should be marked platform. On linux a very small sleep sleep(1) may do, but the real equivalent is sched_yield from sched.h.
Also note both sleep(0) and sched_yield may cause a context switch if there are threads waiting, which makes it often more expensive than a short sleep if that is acceptable.

[MarkMLl]

Thanks for that Thaddy. I'll switch over to Sleep(1) which I think is probably the correct thing under the circumstances, the program has its own yield for the main thread which does a lot of stuff including polling the console for operator input.

MarkMLl

[MarkMLl]

I thought an update might be in order, since it's a good example of the potential problems in multithreaded programming and might be useful to somebody.

I've got a procedure UnitSignal() called by multiple background threads, protected on entry by a critical section.

I've also got CheckUnitSignals() called by the main thread, which checks for queued asynchronous signals (i.e. that a signal should be raised a few mSec in the future).

CheckUnitSignals is protected by the same critical section, since it should obviously not run if another thread has already called UnitSignal().

But CheckUnitSignals() calls UnitSignal(), thus nesting critical section entry.

Specifically, on entry CheckUnitSignals() gets the critical section, then the nested re-get in UnitSignal() is ignored.

On exit UnitSignal() releases the critical section, leaving a short period during which CheckUnitSignals() still thinks it's protected, while in fact it's not.

I don't know whether this is the only issue but it's certainly something that I need to fix.

MarkMLl