Re: [ros-svn] [hbirr] 17538: Fixed a bug in RtlLeaveCriticalSection. We have only to signal the event if someone waits on it.

[Thomas Weidenmueller]

hbirr(a)svn.reactos.com wrote:
>   Fixed a bug in RtlLeaveCriticalSection. We have only
> to signal the event if someone waits on it. 
I believe this change is incorrect. This change introduces the exact
same bug I fixed in r14326.

The reason why this change is incorrect is, that you can't rely on
LockCount. It may be incremented anytime a thread enters a critical
section and waits for it. That's why there's the RecursionCount field,
it is the only information we can rely on to determine whether we're
about to release the lock or not. In this code path we already know that
we left the last recursion, so no matter what the LockCount is at the
moment (depends on how many threads are waiting for the critical
section), we always need to unwait one waiter!

This change most likely will introduce a dead-lock of cygwin.

Best Regards,

Thomas

[Thomas Weidenmueller]

Thomas Weidenmueller wrote:
>   I believe this change is incorrect. This change
> introduces the exact
>  same bug I fixed in r14326.
>  
>  The reason why this change is incorrect is, that you can't rely on
>  LockCount. It may be incremented anytime a thread enters a critical
>  section and waits for it. That's why there's the RecursionCount field,
>  it is the only information we can rely on to determine whether we're
>  about to release the lock or not. In this code path we already know that
>  we left the last recursion, so no matter what the LockCount is at the
>  moment (depends on how many threads are waiting for the critical
>  section), we always need to unwait one waiter!
>  
>  This change most likely will introduce a dead-lock of cygwin. 
Crap, Never mind, your change is correct. I overlooked one minor thing....

Best Regards,
Thomas

[Hartmut Birr]

Hi,

I think that the fix is correct. If a critical section is entered and
left only once. The leave call signals the event. The section is enter
again from two different threads. The first thread gets the section
because the lock count is -1, the second thread must wait on the event
which is already set. Now two different threads owns the section. If a
thread enter the section recursive, only the last leave call will set
the event if someone else waits on the event.

- Hartmut

Thomas Weidenmueller wrote:

>  hbirr(a)svn.reactos.com wrote:
>   
>
> >  Fixed a bug in RtlLeaveCriticalSection. We have
> > only to signal the event if someone waits on it.
> >     
> >  
> I believe this change is incorrect. This change introduces the exact
> same bug I fixed in r14326.
>
> The reason why this change is incorrect is, that you can't rely on
> LockCount. It may be incremented anytime a thread enters a critical
> section and waits for it. That's why there's the RecursionCount field,
> it is the only information we can rely on to determine whether we're
> about to release the lock or not. In this code path we already know that
> we left the last recursion, so no matter what the LockCount is at the
> moment (depends on how many threads are waiting for the critical
> section), we always need to unwait one waiter!
>
> This change most likely will introduce a dead-lock of cygwin.
>
> Best Regards,
>
> Thomas
> _______________________________________________
> Ros-dev mailing list
> Ros-dev(a)reactos.com
> http://reactos.com:8080/mailman/listinfo/ros-dev
>
>
>   
>

[Thomas Weidenmueller]

Hartmut Birr wrote:
>   I think that the fix is correct. If a critical section
> is entered and
>  left only once. The leave call signals the event. The section is enter
>  again from two different threads. The first thread gets the section
>  because the lock count is -1, the second thread must wait on the event
>  which is already set. Now two different threads owns the section. If a
>  thread enter the section recursive, only the last leave call will set
>  the event if someone else waits on the event. 
Yes, you're right. I'm sorry ;)

Best Regards,
Thomas