Hi,

I am having troubles with porting Semaphores in a project under Linux. Under Windows everything works fine, but under Linux my app keeps freezing.

The project is tiOPF v2 (TechInsite Object Persistent Framework). There is a class called TtiPool, that handles a pool of database connections.

When I run the Unit Tests and create a single Lock and the Unlock it, all works fine. If I then iterrate that test by creating 10 locks and then call unlock 10 times, the Unit Tests freeze on the following line with the second iteration:

  if sem_wait(FSemaphore)  0 then
    raise EtiOPFInternalException.Create(cErrorTimedOutWaitingForSemaphore);

* Under Linux, I am using the "pthreads" unit. What is the difference between the cthreads and pthreads unit?
* The calls I am using from pthreads are:
* sem_init() to create a semaphore
* sem_wait() to lock the semaphore
* sem_post() to unlock the semaphore
All the above with a variable of type TSemaphore.

Now, I have to admit, I don't know much about Semaphore's... Anybody know of some good documentation or tutorial on the net? Anything obvious I could be doing wrong?

Regards,
Graeme.

Looking at my code, I might have initialized my semaphore incorrectly.

The Windows portion works, and I tried to port it to Linux as follows...

procedure TtiPool.CreatePoolSemaphore;
begin
  {$IFDEF MSWINDOWS}
  if FSemaphore  0 then
    CloseHandle( FSemaphore ) ;
  FSemaphore := CreateSemaphore( nil, FiMaxPoolSize, FiMaxPoolSize, nil );
  {$ENDIF MSWINDOWS}
  {$IFDEF LINUX}
  sem_destroy( FSemaphore );
  if sem_init(FSemaphore, 0, 1)  0 then
    raise Exception.Create('Failed to create the semaphore');
  {$ENDIF LINUX}
end;

Regards,
Graeme.

When I run the Unit Tests and create a single Lock and the Unlock it, all works fine. If I then iterrate that test by creating 10 locks and then call unlock 10 times, the Unit Tests freeze on the following line with the second iteration:

Sounds like some recursion property is not set.

pthreads is a straight header to the platforms POSIX pthreads library. It is as native to the pthreads lib on that platform as possible, so not 100% portable.

Cthreads is a bit more complicated; Deep in the RTL/FCL and even the compiler we need threading primitives on *nix, but we don't want to import library pthread always, even if we don't use threading.

So we defined a subset of pthreads with uniform typing, and programmed the RTL/FCL against an empty implementation of this interface that is always linked in.

Cthreads is the _full_ implementation (that actually pulls in library pthreads) that is a unit on which no other units have any dependancy. By adding it to the project, the threading support is "armed". The Windows platform doesn't need this construct, since linking to the thread functions is not as potentially problematic, so thread support is installed always.

So language threading, TCriticalSection and syncobjs are mostly built on top of this interface.

Third, there is also the unit IPC, which is more an interface to kernel level IPC.

Looking at unit syncobjs might save you some ifdefs and trouble.

What is FiMaxPoolSize? I assume, it's something like a number of request being allowed inside the semaphore-protected region at once?

So in the linux version only one thread is allowed being there.

I suppose you wrote something like

|for i := 0 to 9 do
|begin
|   Lock;
|   WriteLn ('Ping');
|end {for};
|
|for i := 0 to 9 do
|begin
|   WriteLn ('Pong');
|   Unlock;
|end {for};

In that case you're experiencing the lock-up when the semaphore is being "entered"/locked the second time inside your loop. The counter is zero after the first sem_wait and remains that way, because there's nobody there to call the sem_post() operation. Well, it may seen as a kind of a classical deadlock situation, although there's nothing classical in here. ;)

That's what you're experiencing. Although it might appear similar, a semaphore is *not* a mutex.

What I don't understand is why you don't use the "SyncObjs" unit where almost all the stuff someone ever needs is nicely laid out in a portable and object-oriented way.

> if sem_wait(FSemaphore) 0 then > raise > EtiOPFInternalException.Create(cErrorTimedOutWaitingForSemaphore);

BTW, sem_wait() never returns anything else than zero, so checking is sort of useless here. There's sem_trywait() for that, if you really need to go so low-level.

To the question, what are semaphores. It's quite easily explained: Atomic counters, you can only get past if they're non-zero and in that case the counter is decremented.

Vinzent.

I don't think, semaphores have recursion properties.

Vinzent.

They do, in some implementations.

Michael.

Ok, granted. :) But not in POSIX, AFAICS.

And they were never designed that way. I think, that's why someone invented the mutex. ;)

Vinzent.

Hi Vinzent,

You got me one the right track, I think... :-)

I thought the counter in Semaphores get incremented, but as you stated (and reading some Man Pages), they get decremented.

I then change my code from:

> if sem_init(FSemaphore, 0, 1) 0 then > raise Exception.Create('Failed to create the semaphore');

to ...

> if sem_init(FSemaphore, 0, FiMaxPoolSize) 0 then > raise Exception.Create('Failed to create the semaphore');

Which makes more sense now... FiMaxPoolSize is the maximum number of requests being allow. Having it set to 1, I was only allowed 1, so after the first Lock, the others ended in a deadlock type situation. Hence the second iteration freezing the app.

What I don't understand is why you don't use the "SyncObjs" unit where almost all the stuff someone ever needs is nicely laid out in a portable and object-oriented way.

I didn't write the original Windows code, just tried to port it as is to FPC and get the Unit Tests to pass. I will speak to the original author and see what he thinks of you idea. As long as the code can still compile under Delphi and FPC, I am sure everything should be fine.

I changed it to sem_trywait() which pretty made all my tiPool unit tests pass. For some strange reason I still get a lock-up, but much less now. Say once every 5 iterations of the complete test suite where-as before it was every time on a specific test case. I will now try and implement the timeout work-around as suggested in the IBM article below.

I found the IBM article very handy to explain some information. http://tinyurl.com/n8jq9

Thanks for you time!

Regards,
Graeme.

Looking at unit syncobjs might save you some ifdefs and trouble.

Thanks for the tip. I always get hassled about keeping the IFDEF's as little as possible. I will go browse the SyncObjs unit now.

Regards,
Graeme.

hmm, so a mutex can be recursive, if the type is set correctly, which is not supportet on all platforms implementing pthread (man-pages of pthread).

I vote for more pascal based versions of TMutex, TSemaphore and TEvent, that behaves just like the ones in Delphi and use the system specific functions (like pthread stuff) only internally in their simplest/most portable form.

Currently, I have the problem to get a working TEvent:
- The one in syncobjs returns wrError, when it's waitfor is called with anything else tah infinite timeout
- same goes for rtlbasicevent (cthreads)
- the rtlEvent works even with timeout (with my patch posted on mantis), but this one clears the signaled state, when wait is called. IOW. if I call waitfor on the allready signalled event, it won't return before timeout. Also it doesn't inform the caller, wheter it returnt due to signalled or to timeout ..

regards
Burkhard

Burkhard Carstens wrote

I vote for more pascal based versions of TMutex, TSemaphore and TEvent, that behaves just like the ones in Delphi and use the system specific functions (like pthread stuff) only internally in their simplest/most portable form.

I'd vote for a more abstract version. :) The more or less basic things needed in multi-threaded programing are

- semaphore
- persistent signal
- transient signal ("pulse")
- event

There's some more higher level stuff (like broadcasts, for example), but that's about all you usually need. Currently we have the semaphore (more or less as SyncObjs.tCriticalSection), the persistent and the transient signal (SyncObjs.tEvent), although without timeouts (at least when trying to be portable), the event is somewhat implemented, but IIRC it misses a "Toggle" property, which can be used for simple intertask communication ("Now I'm blocking, you're on.").

Currently, I have the problem to get a working TEvent: - The one in syncobjs returns wrError, when it's waitfor is called with anything else tah infinite timeout

Yes, that's not implemented, although it should be easily added by using condition variables of pthreads.

- the rtlEvent works even with timeout (with my patch posted on mantis), but this one clears the signaled state, when wait is called.

*oh* Before the wait? That's not good. This cries for race conditions and subtle deadlock situations.

Vinzent.

..which currently doesn't work, unless someone applies my patch (or a better one) ..

thread synchronization uses this and takes care of it by calling RTLeventstartwait which locks the associated mutex. So I don't expect a race condition there, but don't know about other places.

As far as I just read code and comments, it does it for Windows to get Unix behaviour, although I don't see the point here, because this just forces the race condition the POSIX-Threads implementation actually tries to circumvent with this associated mutex.

I'm not even sure, if Unix actually clears the event. The man-pages don't seem to indicate so.

Vinzent.

I don't think so. In unix, the mutex is locked, which ensures, there is not setevent between startwait and actual call to wait, because the call to wait seems to reset the cond, then releases the mutex and starts waiting. In windows, this doesn't matter (resetting the event on startwait), because it preserves the signaled state of the event. The wait call just returns immediately, if the event is allready signaled. To me, this is fine. As long as rtlevent is used with this in mind, it should give a consistent behaviour on windows and linux. (see classes.inc thread synchronization)

I'm not even sure, if Unix actually clears the event. The man-pages don't seem to indicate so.

It does, at least with the timedwait. I tested that extensively.

Burkhard

[..]

... As long as rtlevent is used with this in mind, it should give a consistent behaviour on windows and linux.

"this in mind" means: The only way how rtlevent should be used is:
1. call rtlstartwait
2. start the code (e.g. start/resume a thread), that signals the event.
3. call rtlwaitevent

Well, in that case either I am totally stupid (won't be news, anyway) or I must have different source code:

From "rtl/unix/cthreads.inc":

|procedure intRTLEventStartWait(AEvent: PRTLEvent);
|var p:pintrtlevent;
|
|begin
|  p:=pintrtlevent(aevent);
|  pthread_mutex_lock(@p^.mutex);
|end;
|
|procedure intRTLEventWaitFor(AEvent: PRTLEvent);
|var p:pintrtlevent;
|
|begin
|  p:=pintrtlevent(aevent);
|  pthread_cond_wait(@p^.condvar, @p^.mutex);
|  pthread_mutex_unlock(@p^.mutex);
|end;

Which is pretty much what you described and if pthread_cond_wait() works as described, there's no race condition here.

But now "rtl/win/systhrd.inc":

|procedure intRTLEventStartWait(AEvent: PRTLEvent);
|begin
|  { this is to get at least some common behaviour on unix and win32:
|    events before startwait are lost on unix, so reset the event on
|    win32 as well }
|  ResetEvent(THANDLE(AEvent));
|end;
|
|procedure intRTLEventWaitFor(AEvent: PRTLEvent);
|const
|  INFINITE=dword(-1);
|begin
|  WaitForSingleObject(THANDLE(AEvent), INFINITE);
|end;

I don't see any locking in intRTLEventStartWait(), but I see the signal being cleared in an unprotected way. So the signal can still get set between the calls to intRTLStartWait() and intRTLEventWaitFor() which makes the clearing quite useless.

In unix, the mutex is locked, which ensures, there is not setevent between startwait and actual call to wait, because the call to wait seems to reset the cond, then releases the mutex and starts waiting.

Yes, and now I seem to understand why. Unix only implements transient signals. So if there's noone waiting on the signal, it gets lost. Windows OTOH seems to implement persistent signals. That's all the difference.

In windows, this doesn't matter (resetting the event on startwait), because it preserves the signaled state of the event.

It preserves the signalled state? By resetting it? I don't think so.

AFAICS, it resets the signal before actually starting to wait for it. So it still can sneak in if the timing is right. That's what I'd call a race condition.

The wait call just returns immediately, if the event is allready

signaled.

To me, this is fine. As long as rtlevent is used with this in mind, it should give a consistent behaviour on windows and linux. (see classes.inc thread synchronization)

You mean code like:

|if timeout>0 then
|  begin
|    RtlEventStartWait(SynchronizeTimeoutEvent);
|    RtlEventWaitFor(SynchronizeTimeoutEvent,timeout);
|  end
|  else
|    RtlEventResetEvent(SynchronizeTimeoutEvent);

So where's the code that makes sure that the thread signalling the event is (only) resumed between the two calls?

> I'm not even sure, if Unix actually clears the event. The man-pages > don't seem to indicate so. It does, at least with the timedwait. I tested that extensively.

Yes, and now I think we both know why. I seem to repeat myself, but it might be important: The difference is all between implementing transient and persistent signals.

Vinzent.

Great. ;( But there's a flaw in this: If I'd actually know that thread A had been executed rtlstartwait() before thread B tries signalling the condition, I wouldn't need the whole .

Vinzent Höfler wrote:

I didn't understand and follow the whole thread but please submit a bug report if something in FPC needs to be fixed :)

Florian Klaempfl wrote

I didn't understand and follow the whole thread but please submit a bug report if something in FPC needs to be fixed :)

Well, I'll do as soon as I'm sure if there's a bug. :)

Currently all I see is a subtle semantic difference between Windows- and Unix-Event's implementation.

So for now all I can say is that this should either

a) be documented,
b) changed to a common semantic behaviour, or
c) ignored, because I'm just too stupid.

Vinzent.

Vinzent Höfler wrote

Currently all I see is a subtle semantic difference between Windows- and Unix-Event's implementation.

AFAICS, it's as close as it can get. You mean subtle as in, unimportant, or as in, possibly fatal ? :-) If possibly fatal, describe when/how.

So for now all I can say is that this should either a) be documented,

*Could* be a good idea, yes ;-).

b) changed to a common semantic behaviour, or

They're both more or less 'transient' now.

I am not sure how to emulate windows behavior on unix, although that behavior is easier to use I think, so would be desirable.

Micha

You want to be sure, a thread started in step 2 has finished, before you proceed to step 4 ..

btw. this is not TEvent, we are talking about, it's rtlevent .. TEvent (in synconjs) is implemented differently, and should work like delphi/ windows TEvent, except that it is not capable of timedwait ..

I am sure, you're not ;-)

here, the mutex is unlocked, and wait state is entered, If timeout occours or cond gets signalled, mutex is locked again and wait returns

Yea, but as it's windows, that doesn't matter. setting the signal between startwait and waitfor doesn't hurt, because windows doesn't reset it before waiting. Resetting it in startwait just ensures, that we have roughly the same behaviour like in unix, i.e. clear any previously set signal and catch anything that gets signalled between the call to startwait and return of waitfor...

right.

I mean: it doesn't reset it on the waitforsingleobject call, at least not *before* it starts waiting ..

You are right! Looks like this is a bug! However, in unix, this would't hurt anyway, as RtlEventWaitFor with timeout allways returns immediately (mantis #7196) ;-)
Guess, no one tried to use checksynchonize with timeout 0 yet, otherwise, we'd have a bugreport :-)

Agreed!

Burkhard

Ahh, yes. Now I see where you're going. What you're saying is that RTLEvent can't be used asynchronously (i.e. from different threads signalling each other) in a consistent way.

Well, *that* should be documented. I thought, trying to achieve exactly that behaviour was the whole idea.

btw. this is not TEvent, we are talking about, it's rtlevent .. TEvent (in synconjs) is implemented differently, and should work like delphi/ windows TEvent, except that it is not capable of timedwait ..

Well, if I'm looking at it correctly that's only because its implementation is based on the POSIX semaphore. It could be using the phtread_mutex stuff instead. The pthread-primitives are used anyway, so I don't see problems with dependencies on libpthread/libc or so...

Hmm, *looking closer* ... are there any differences between Darwin/BSD/Linux/Solaris code at all? AFAICS, all these implementations could be merged into a single one for Unix/POSIX targets.

Vinzent.

If used incorrectly, i.e. asynchronously.

Depends on the view point. Doing two consecutive waits() might do very different things. :)

So I'd say, the behaviour and intended use should definitely be documented.

I am not sure how to emulate windows behavior on unix, although that behavior is easier to use I think, so would be desirable.

You'd have to create an own abstraction about the useful primitives. That's what I was initially talking about. :) Dunno, if that's good for the RTL, because you'd probably loose /some/ speed on the way...

Vinzent.

My idea was to use a rtlevent additionally and only when waitfor is called with timeout AND event is not signalled yet.

Hmm, *looking closer* ... are there any differences between Darwin/BSD/Linux/Solaris code at all? AFAICS, all these implementations could be merged into a single one for Unix/POSIX targets.

Don't know, maybe. However, we use PTHREAD_MUTEX_RECURSIVE_NP, where NP stands for NotPortable.. don't know which platforms support it and which not.
That's why I voted for implementing most of the functionality in pascal and use pthread primitives only in their basic (i.e. mutex in non-recursive) form..

Burkhard

Why additionally? Do I miss something here? If done the proper way, everything in SyncObjs should be based on the basic RTL stuff. This would make the SyncObjs unit system independent with clearly defined semantics on all targets.

Of course, the original implementation (I suppose it came from Delphi?) was very windows-centric and made use of the available Windows-primitives directly. So maybe for Win32 it should even be kept this way, if not for anything else, somebody may say, for performance reasons at least.

Well, those flags are somhow there, but there marked "XSI" in the OpenGroup's current specification, so probably most of our targets implement it - but it's still an extension.

That's why I voted for implementing most of the functionality in pascal and use pthread primitives only in their basic (i.e. mutex in non-recursive) form.

Hmm. So we'd need a mutex inside a mutex. Now I know why they call it recursive. ;) So it'll be something like that:

function Recursive_Mutex.Lock : ...;
begin
    // Lock mutex inside mutex.
    self.Owner_Check_Lock.Lock;
    // Owned by current thread?
    if CurrentThreadId  self.ThreadId then
    begin
       // Nope! Get the hell outta here.
       self.Owner_Check_Lock.Unlock;
       exit (NOT_OWNED);
    end {if};
    // Now try locking the real mutex.
    if pthread_mutex_lock (self...) = 0 then
    begin
       self.Count := self.Count + 1;
       self.Owner_Check.Unlock;
       exit (SUCCESS);
    end {if};
    self.Owner_Check.Unlock;
    exit (FAILURE);
end {Mutex.Lock};

Something like that. Don't nail me on that, it's quite late and the heat is still killing me. ;)

Well, a simple spinlock would even be better here for performance reasons, because first the locking time should be quite small, and second usually this code will be called in a sequential manner from within the same thread anyway. This would make the lock always being in an unlocked state under all sane circumstances.

Maybe even some overflow check in case someone tries to lock the mutex more than 2**32 times in a row? ;)

Same for unlock etc.

Vinzent.

I knew it. Of course this is wrong. We can only lock the mutex once, because we assume it's non-recursive, that was the whole point. So we should *first* check the count and then may lock/unlock the mutex accordingly.

Vinzent.

yea, this stuff is allways really brain cracking ;-) .. heat kills me, too, so I will re-read these mails tomorrow and think about it again.. good night
Burkhard

Vinzent Höfler wrote

because we assume it's non-recursive, that was the whole point. So we should *first* check the count and then may lock/unlock the mutex accordingly.

Note that these two actions must be atomic. This is what the TryLock is for.

Micha

Oh, really?

This is what the TryLock is for.

No, that's what the "Owner_Check_Lock" in my posted code would be for in the first place. But you're right, TryLock makes it possibly easier to detect/handle failures.

So the code from

   // Now try locking the real mutex.

should actually be more like:

| // First encounter? So lock it actually.
| if self.Count = 0 then
| begin
| // This should *always* succeed, because we're the owning | // thread (we've just checked it, right?) and we didn't lock it | // yet according to "Count".
| // Thinkable ways how this can fail is if somebody has direct | // access to the actual pthread mutex inside here or we | // fuc^Wmessed up the counter somehow ...
| if pthread_mutex_trylock (self...) 0 then | // Our real mutex couldn't be locked!? This *is* strange. | exit (FAILURE);
| end {if};
|
| // Alright, we got it. Mutex is locked (or at least, it *should* be | // already), so count up.
| self.Count := self.Count + 1;
| self.Owner_Check.Unlock;
| exit (SUCCESS);

A pthread_mutex_trylock alone wouldn't actually help on a non-recursive "real" mutex. Because: if it's already locked I still don't know if it belongs to the current thread, but precisely that knowledge is needed to implement the recursive one on top of a non-recursive.

Vinzent.

Ehm, no. Got confused a bit :-). Reread the thread, and think your latest implementation as posted here is ok.

Micha

Ehm, yes. I was being ironic here. Of course, the action of checking the counter and locking/unlocking the associated mutex must be atomic.

Got confused a bit :-). Reread the thread, and think your latest implementation as posted here is ok.

No, it's not. As usually I should sit down with pencil, paper, a cup of coffee and a cigarette before writing code. Anything else is hacking and takes more time and changes. (Why the hell don't I listen to myself?)

Despite the fact that some minor details may be still missing, like setting the owner's ThreadId, for example, this implementation has one very major flaw. It doesn't suspend the calling thread if it's not already owned by it and if it is, it doesn't do that either, becaus it shouldn't. That makes it kind of useless, I think. :)

Instead of exiting with (NOT_OWNED) the mutex should be locked, of course. But before doing that, the Owner_Check_Lock must be released. And now I start smelling a race condition. Back to the drawing board, I guess.

Vinzent.

Vinzent Hoefler wrote

> Ehm, no. Ehm, yes. I was being ironic here. Of course, the action of checking the counter and locking/unlocking the associated mutex must be atomic.

But here they are not associated, they're protected by owner_lock, as you said.

> Got confused a bit :-). Reread the thread, and think your > latest implementation as posted here is ok. No, it's not. As usually I should sit down with pencil, paper, a cup of

I mean the idea for the recursive locking is ok. I was confused by the whole 'owning' part. Locks don't own threads in my mind, so I assumed you got that right. Alas, no :-). Also problematic is posting only partly updates to a function. Then we forget the other, also critical part.

function Recursive_Mutex.Lock:...;
begin
   // Owned by current thread?
   if CurrentThreadId  self.ThreadId then
   begin
      result := pthread_mutex_lock (self...);
      if result  0 then exit;
      self.ThreadId := CurrentThreadId;
      self.Count := 1;
   end else begin
      Inc(self.Count);
   end;
   result := SUCCESS;
end {Mutex.Lock};
function Recursive_Mutex.Unlock: ...;
begin
  if CurrentThreadId  self.ThreadId then
    exit(ENOTOWN);
  assert(self.Count > 0);
  dec(self.Count);
  if self.Count = 0 then
  begin
    self.ThreadId := 0; // a threadid that does not occur in system
    pthread_mutex_unlock(self...);
  end;
  result := SUCCESS;
end;

I don't see the need for Owner_Check_Lock anymore ? :-)

Micha

* It's early, and I didn't dink enough coffee yet, so excuse me, if I overlook something ;-)

some points:

* In windows, I never used mutexes, as I didn't see the point. Usually, CriticalSections do what I want, except for IPC ... So I don't know if windows mutexes are recursive.
However, IMO a CriticalSection should be recursive, so I stick to it here

* pthread-Mutexes may not be used asynchronously (signal handlers are mentioned in the man page), so if we take that as basic rule, things get pretty simple: the same code is never executed simultanously from the same thread.

* rtleventStartWait should have a counterpart rtlEventDoneWait, which unlocks the internal mutex of a rtlevent.

* rtlWaitEvent should get a parameter telling it whether to call rtlEventDoneWait or not. (currently, it simply does it)

* protect any method of a TCriticalSection with a internal critical section (a simple, non-recursive form should do the job)

TCriticalSection.Enter;
  procedure internalEnter;
  begin
    OwnerThread:=GetCurrentThreadID;
    fLockCount:=1;
    ExternelLock.lock;
  end;
begin
  internal.lock;
  if OwnerThread = 0 then begin //not locked yet
    internalEnter;
  end
  else begin //allready locked
    if OwnerThread=GetCurrentThreadID then //we are the owner
      inc(fLockCount)
    else begin //other thread owns the cs
      rtlEventStartWait(fExternalLock);
      internallock.unlock;
      rtlEventWaitFor(fExternalLock,DontAutoReleaseRTLEventMutex);
      internalLock.lock;
      rtlEventDoneWait(fExternalLock);
      internalEnter;
    end;
  end;
  internal.unlock;
end;
TCriticalSection.Leave;
  procedure internalEnter;
  begin
    OwnerThread:=GetCurrentThreadID;
    fLockCount:=1;
    ExternelLock.lock;
  end;
begin
  internal.lock;
  try
    if OwnerThread  GetCurrentThreadID then begin //we are not allowed
      //to leave, because we don't own it!not locked yet
      raise exception.create('leave on a foreign critical section');
    end
    else begin //we are the owner
      if fLockCount = 0 then begin
        raise exception.create('leave on a unlocked section');
      end
      else begin
        dec(fLockCount);
        if fLockCount=0 then rtlSetEvent(fExternalLock);
      end;
    end;
  finally
    internal.unlock;
  end;
end;

* just a quick draft. This one should be pretty much the same as a recursive mutex, i think ..

* have to leave now, bbl. Maybe we meet in IRC later and/or setup a wiki page about this topic to collect and discuss proposals

Regards,
Burkhard

You have to prevent that 2 concurrent threads call lock simultanously, so you have to take an internal lock before touching internal vars like self.ThreadID ?

Burkhard

Burkhard Carstens wrote

> I don't see the need for Owner_Check_Lock anymore ? :-) You have to prevent that 2 concurrent threads call lock simultanously,

No, why?

so you have to take an internal lock before touching internal vars like self.ThreadID ?

No, why ? This is a common misconception I think. (Multi-threaded) Programs are about state, not about touching internal variables.

Micha

[...]
coffee, pencil and stuff ;-)
off course, the local procedure "internalEnter" in Leave is a copy'n'paste rubbish. Just remove ..

TCriticalSection.Leave;
begin
  internal.lock;
  try
    if OwnerThread  GetCurrentThreadID then begin //we are not
       // allowed to leave, because we don't own it!
       raise exception.create('leave on a foreign critical section');
    end
    else begin //we are the owner
      if fLockCount = 0 then begin
        raise exception.create('leave on a unlocked section');
      end
      else begin
        dec(fLockCount);
        if fLockCount=0 then rtlSetEvent(fExternalLock);
      end;
    end;
  finally
   internal.unlock;
  end;
end;

No. That's not a problem. See the explanation below.

Perhaps the line "if result 0 then exit;" may be kind of superfluous, or even dangerous, because it would return from the lock operation without actually locking it, but well - if it goes wrong here, we're screwed anyway. At least the caller could check, if it did.

so you have to take an internal lock before touching internal vars like self.ThreadID ?

First I thought that, too. But after looking at and thinking about it a while, the race you mention could only occur if the *same* thread would call Lock() simultaneously, and that can't happen.

In the case two different threads enter here, they may both still see "0" and are trying to continue then, but only one of them will acquire the mutex sucessfully, leaving the other suspended. Once the suspended thread continues, it actually owns the mutex, so everything's ok then.

Ok, there's a glitch: The read and write of self.ThreadId is required to be atomic, so that a thread entering may either see "0" or the owner's thread id when checking, otherwise it could be possible, it sees its own thread_id due to a partial update and *then* it goes really wrong.

Vinzent.

Yes. That's what would have made the whole operation atomic.

I mean the idea for the recursive locking is ok.

Well, in the end it's functionality that counts. Ideas I got. Lots.

I was confused by the whole 'owning' part. Locks don't own threads in my mind, so I assumed you got that right.

Yes, that's right, locks don't own threads. But threads own locks. :)

Alas, no :-). Also problematic is posting only partly updates to a function. Then we forget the other, also critical part.

Yepp. It wasn't yet meant to be committed to svn, you know? ;)

function Recursive_Mutex.Lock:...;

[...]

function Recursive_Mutex.Unlock: ...;

[...]

I don't see the need for Owner_Check_Lock anymore ? :-)

Yes, that looks about right. ;-) Except perhaps for the possible glitch I mentioned in my other mail. Don't know, if we can ignore it for now or simply assume that (up to) 32-Bit-variables always *are* atomic types.

Vinzent.

To prevent this very rare but indeed possible case, just use interlockedexchange to assign anything to self.threadid ?

Beside this, I like this implementation! This way, we have a recursive mutex, without depending on pthread recursive extension.

Burkhard

Vinzent Hoefler wrote

Ok, there's a glitch: The read and write of self.ThreadId is required to be atomic, so that a thread entering may either see "0" or the owner's thread id when checking, otherwise it could be possible, it sees its own thread_id due to a partial update and *then* it goes really wrong.

It's not a glitch but an assumption I'd say. The compiler devels should be able to answer whether we can assume this, or not ?

Micha

Well, it will become a glitch, if the assumption about the atomic read/ write turns out to be wrong. ;-)

The compiler devels should be able to answer whether we can assume this, or not ?

Well, I suppose so. But in the end it depends on the target architecture, not the compiler in general.

That's BTW, one of the reasons I'd like to have a Pascal's equivalent of Ada's "Pragma Atomic(...)" and "Pragma Volatile(...)" for certain types and variables.

Vinzent.