CriticalSection bug

2006-12-25
2013-04-08
  • Nobody/Anonymous

    While browsing your project's source code, I just noticed a bug in CriticalSection::unlock():

        void unlock()
        {
            ::LeaveCriticalSection( & itsCs );
            itsIsLocked = false;
        }

    should be:

        void unlock()
        {
            itsIsLocked = false;
            ::LeaveCriticalSection( & itsCs );
        }

    Otherwise CriticalSection::isLocked() can wrongly return false while the CS is locked.

     
    • andrew7

      andrew7 - 2006-12-26

      Note that ::LeaveCriticalSection does not block, so we are examining a rare case.

      It is possible to have a switch from the unlock code in a few places:

      void unlock()
      {
      #1
      ::LeaveCriticalSection( & itsCs );
      #2
      itsIsLocked = false;
      #3
      }

      Switching away for #1 or #3 causes no problem.   If we switch away while a thread is at #2,  then rarely itsIsLocked is true, but the ::LeaveCriticalSection was called.  The thread that gets itsIsLocked is true must handle this condition anyhow. So I don't see a big impact. 

      If we take your suggested code, then rarely itsIsLocked is false, but the ::LeaveCriticalSection was not called.  So another thread then calls lock, and has to wait until the original thread resumes and does the ::LeaveCriticalSection.  Note that itsIsLocked might be false now, but another thread could jump in and enter the critical section before you anyhow.
      So I don't see a big impact.

      So I suggest we just leave the code as it is.

      best regards, Andrew

      p.s.  Perhaps this is what you meant, because your change sets itsIsLocked even sooner, before the ::LeaveCriticalSection happens.  That change would make it more likely that the ::LeaveCriticalSection has not happened yet. And the stated problem was "CriticalSection::isLocked() can wrongly return false while the CS is locked"  That is actually the slight problem with your "should be", as I understand it.

       

Log in to post a comment.

Get latest updates about Open Source Projects, Conferences and News.

Sign up for the SourceForge newsletter:

JavaScript is required for this form.





No, thanks