Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Orphaned ReaderWriterLockSlim #12107

Closed
nooogle opened this issue Feb 22, 2019 · 3 comments
Closed

Orphaned ReaderWriterLockSlim #12107

nooogle opened this issue Feb 22, 2019 · 3 comments

Comments

@nooogle
Copy link

nooogle commented Feb 22, 2019

EnterReadLock called on a ReaderWriterLockSlim inside a paint handler never return when this scenario occurs:

  1. Worker thread claims the lock for Write access.

  2. GUI thread [concurrently] attempts to claim the lock for Write access; it can't, so InternalWaitOne is called. The message queue is found to have a paint message so the OnPaint method gets called.

  3. OnPaint tries to claim the lock for Read access. If the worker thread still has the Write lock then another InternalWaitOne is called but this never returns and the GUI locks, even though the worker thread releases its Write lock.

The problem does not occur if OnPaint is changed to claim Write access; this suggests some anomalous behaviour with EnterReadLock.

(I appreciate this is all very bad design, it is lifted from some legacy code I have to work on. But I don't expect that EnterReadLock should never return when the lock becomes unclaimed by any thread).

I've made a simple WinForms app to demonstrate this: https://github.com/nooogle/SlimLockTest

Breaking the app when it has locked shows this call stack on the GUI:

mscorlib.dll!System.Threading.WaitHandle.WaitOne	Unknown
System.Core.dll!System.Threading.ReaderWriterLockSlim.WaitOnEvent	Unknown
System.Core.dll!System.Threading.ReaderWriterLockSlim.TryEnterReadLockCore	Unknown
System.Core.dll!System.Threading.ReaderWriterLockSlim.TryEnterReadLock	Unknown
System.Core.dll!System.Threading.ReaderWriterLockSlim.EnterReadLock	Unknown
>	SlimLockTest.exe!SlimLockTest.SharedResource.LockRead Line 10	C#
SlimLockTest.exe!SlimLockTest.Form1.OnPaint Line 41	C#
System.Windows.Forms.dll!System.Windows.Forms.Control.PaintWithErrorHandling	Unknown
System.Windows.Forms.dll!System.Windows.Forms.Control.WmPaint	Unknown
System.Windows.Forms.dll!System.Windows.Forms.Control.WndProc	Unknown
System.Windows.Forms.dll!System.Windows.Forms.Form.WndProc	Unknown
System.Windows.Forms.dll!System.Windows.Forms.NativeWindow.DebuggableCallback	Unknown
[Native to Managed Transition]	
[Managed to Native Transition]	
mscorlib.dll!System.Threading.WaitHandle.InternalWaitOne	Unknown
mscorlib.dll!System.Threading.WaitHandle.WaitOne	Unknown
System.Core.dll!System.Threading.ReaderWriterLockSlim.WaitOnEvent	Unknown
System.Core.dll!System.Threading.ReaderWriterLockSlim.TryEnterWriteLockCore	Unknown
System.Core.dll!System.Threading.ReaderWriterLockSlim.TryEnterWriteLock	Unknown
System.Core.dll!System.Threading.ReaderWriterLockSlim.EnterWriteLock	Unknown
SlimLockTest.exe!SlimLockTest.SharedResource.LockWrite Line 9	C#
SlimLockTest.exe!SlimLockTest.Form1.timerLockTest_Tick Line 55	C#
System.Windows.Forms.dll!System.Windows.Forms.Timer.OnTick	Unknown
System.Windows.Forms.dll!System.Windows.Forms.Timer.TimerNativeWindow.WndProc	Unknown
System.Windows.Forms.dll!System.Windows.Forms.NativeWindow.DebuggableCallback	Unknown
[Native to Managed Transition]	
[Managed to Native Transition]	
System.Windows.Forms.dll!System.Windows.Forms.Application.ComponentManager.System.Windows.Forms.UnsafeNativeMethods.IMsoComponentManager.FPushMessageLoop	Unknown
System.Windows.Forms.dll!System.Windows.Forms.Application.ThreadContext.RunMessageLoopInner	Unknown
System.Windows.Forms.dll!System.Windows.Forms.Application.ThreadContext.RunMessageLoop	Unknown
SlimLockTest.exe!SlimLockTest.Program.Main Line 19	C#
@filipnavara
Copy link
Member

filipnavara commented Feb 22, 2019

That is a general issue with any re-entrant lock on COM STA threads. You are not supposed have any locks in the code paths that are pumped by CoWaitForMultipleObjects and that includes the paint handler. Best case you will get deadlocks, but in many cases you will get random stack overflows that are hard to debug or application states that seemingly just cannot happen (until you realize that the locks pump certain messages).

@nooogle
Copy link
Author

nooogle commented Feb 25, 2019

That is a general issue with any re-entrant lock on COM STA threads. You are not supposed have any locks in the code paths that are pumped by CoWaitForMultipleObjects and that includes the paint handler. Best case you will get deadlocks, but in many cases you will get random stack overflows that are hard to debug or application states that seemingly just cannot happen (until you realize that the locks pump certain messages).

I'm happy for this to be closed since it happens due to a combination of several bad practices. However, I don't know if this explains why EnterWriteLock always works while EnterReadLock always (eventually) locks up. The stack never overflows, the call stack is identical every time, the worker thread is still happily running (and claiming then releasing its Write lock), and other than the time it takes to break there's nothing random happening. It really seems that there's a logic glitch somewhere that prevents the read lock attempt from ever getting notified that lock is now free. I've even had the worker thread exit several seconds after the GUI thread orphan has occurred, but it has no effect.

@msftgits msftgits transferred this issue from dotnet/coreclr Jan 31, 2020
@msftgits msftgits added this to the Future milestone Jan 31, 2020
@maryamariyan maryamariyan added the untriaged New issue has not been triaged by the area owner label Feb 26, 2020
@jeffschwMSFT jeffschwMSFT removed the untriaged New issue has not been triaged by the area owner label Apr 9, 2020
@kouvel
Copy link
Member

kouvel commented Sep 7, 2022

ReaderWriterLockSlim has several issues with reentrancy and it doesn't appear to be a simple fix. Deferring to #8710.

@kouvel kouvel closed this as completed Sep 7, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Oct 7, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

6 participants