-
Notifications
You must be signed in to change notification settings - Fork 643
Replace HandleDictionary lock with a critical section under Windows #1817
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
Changes from 3 commits
0792177
3b33a3f
5a6be16
a97704c
2388e06
d0798ca
88b8ad5
b7720dc
e2a6713
a0655df
e8ab7d2
7f71a1d
4e33f2f
87cc98c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,7 @@ | ||
| using System; | ||
| using System.Collections.Generic; | ||
| using System.Threading; | ||
| using System.Runtime.InteropServices; | ||
| #if THROW_OBJECT_EXCEPTIONS | ||
| using System.Collections.Concurrent; | ||
| #endif | ||
|
|
@@ -16,7 +17,7 @@ internal static class HandleDictionary | |
| #endif | ||
| internal static readonly Dictionary<IntPtr, WeakReference> instances = new Dictionary<IntPtr, WeakReference> (); | ||
|
|
||
| internal static readonly ReaderWriterLockSlim instancesLock = new ReaderWriterLockSlim (); | ||
| internal static readonly IPlatformLock instancesLock = PlatformLock.Create (); | ||
|
|
||
| /// <summary> | ||
| /// Retrieve the living instance if there is one, or null if not. | ||
|
|
@@ -212,4 +213,124 @@ internal static void DeregisterHandle (IntPtr handle, SKObject instance) | |
| } | ||
| } | ||
| } | ||
|
|
||
|
|
||
| /* | ||
| * This is a (hopefully) temporary fix for issue #1383. | ||
| * | ||
| * https://github.com/mono/SkiaSharp/issues/1383 | ||
| * | ||
| * On Windows, .NET locks are alertable when using the STA threading model and can | ||
| * cause the Windows message loop to be dispatched (typically on WM_PAINT messages. | ||
| * This can lead to re-entrancy and a deadlock on the HandleDictionary lock. | ||
| * | ||
| * This fix replaces the ReaderWriteLockSlim instance on Windows with a native Win32 | ||
| * CRITICAL_SECTION. | ||
| */ | ||
|
|
||
| /// <summary> | ||
| /// Abstracts a platform dependant lock implementation | ||
| /// </summary> | ||
| interface IPlatformLock | ||
| { | ||
| void EnterReadLock (); | ||
| void ExitReadLock (); | ||
| void EnterWriteLock (); | ||
| void ExitWriteLock (); | ||
| void EnterUpgradeableReadLock (); | ||
| void ExitUpgradeableReadLock (); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Helper class to create a IPlatformLock instance depending on the platform | ||
| /// </summary> | ||
| static class PlatformLock | ||
| { | ||
| public static IPlatformLock Create () | ||
| { | ||
| if (RuntimeInformation.IsOSPlatform (OSPlatform.Windows)) | ||
| return new NonAlertableWin32Lock (); | ||
| else | ||
| return new ReadWriteLock (); | ||
| } | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Non-Windows platform lock uses ReaderWriteLockSlim | ||
| /// </summary> | ||
| class ReadWriteLock : IPlatformLock | ||
| { | ||
| public void EnterReadLock () => _lock.EnterReadLock (); | ||
| public void ExitReadLock () => _lock.ExitReadLock (); | ||
| public void EnterWriteLock () => _lock.EnterWriteLock (); | ||
| public void ExitWriteLock () => _lock.ExitWriteLock (); | ||
| public void EnterUpgradeableReadLock () => _lock.EnterUpgradeableReadLock (); | ||
| public void ExitUpgradeableReadLock () => _lock.ExitUpgradeableReadLock (); | ||
|
|
||
| ReaderWriterLockSlim _lock = new ReaderWriterLockSlim (); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Windows platform lock uses Win32 CRITICAL_SECTION | ||
| /// </summary> | ||
| class NonAlertableWin32Lock : IPlatformLock | ||
| { | ||
| public NonAlertableWin32Lock () | ||
| { | ||
| _cs = Marshal.AllocHGlobal (Marshal.SizeOf<CRITICAL_SECTION> ()); | ||
| if (_cs == IntPtr.Zero) | ||
| throw new OutOfMemoryException ("Failed to allocate memory for critical section"); | ||
|
|
||
| InitializeCriticalSectionEx (_cs, 4000, 0); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should these calls check the result and throw using
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've put in a check on the AllocHGlobal to throw an out of memory exception. I'm not sure what conditions InitCritSec would fail (I've never seen one fail) so not sure what exception to throw.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I mean the InitializeCriticalSectionEx may return false and have an error.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, I understand that... I just wasn't sure what exception to throw? Suggestions?
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looking at people's code online, seems that they just use void and thus never care. I wonder why. Maybe it never fails?
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I looked in CoreRT and Sqlite. So I guess just ignore for now... I saw a comment on
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not 100% sure, but I suspect that InitializeCriticalSection just initializes the fields of the CRITICAL_SECTION structure... so it could be that it only fails if you pass it bad parameters like a null pointer for example. |
||
| } | ||
|
|
||
| ~NonAlertableWin32Lock () | ||
| { | ||
| if (_cs != IntPtr.Zero) { | ||
| DeleteCriticalSection (_cs); | ||
| Marshal.FreeHGlobal (_cs); | ||
| } | ||
| } | ||
|
|
||
| IntPtr _cs; | ||
|
|
||
| void Enter () | ||
| { | ||
| EnterCriticalSection (_cs); | ||
| } | ||
|
|
||
| void Leave () | ||
| { | ||
| LeaveCriticalSection (_cs); | ||
| } | ||
|
|
||
| public void EnterReadLock () { Enter (); } | ||
| public void ExitReadLock () { Leave (); } | ||
| public void EnterWriteLock () { Enter (); } | ||
| public void ExitWriteLock () { Leave (); } | ||
| public void EnterUpgradeableReadLock () { Enter (); } | ||
| public void ExitUpgradeableReadLock () { Leave (); } | ||
|
|
||
| [StructLayout (LayoutKind.Sequential)] | ||
| public struct CRITICAL_SECTION | ||
| { | ||
| public IntPtr DebugInfo; | ||
| public int LockCount; | ||
| public int RecursionCount; | ||
| public IntPtr OwningThread; | ||
| public IntPtr LockSemaphore; | ||
| public UIntPtr SpinCount; | ||
| } | ||
|
|
||
| [DllImport ("Kernel32.dll", SetLastError = true)] | ||
| [return: MarshalAs (UnmanagedType.Bool)] | ||
| static extern bool InitializeCriticalSectionEx (IntPtr lpCriticalSection, uint dwSpinCount, uint Flags); | ||
| [DllImport ("Kernel32.dll")] | ||
| static extern void DeleteCriticalSection (IntPtr lpCriticalSection); | ||
| [DllImport ("Kernel32.dll")] | ||
| static extern void EnterCriticalSection (IntPtr lpCriticalSection); | ||
| [DllImport ("Kernel32.dll")] | ||
| static extern void LeaveCriticalSection (IntPtr lpCriticalSection); | ||
| } | ||
|
|
||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would this be worth making into a public (maybe hidden) API such that if you only ever have a single threaded app that never needs to look things up, you can just skip locks altogether and don't pay the price of locking?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You still need some sort of locking because the HandleDictionary can be called from the GC thread.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On further thought, having this as a public API might be a really good idea. It could also be a good alternative to including the critical section implementation at all. If I could simply plugin my own sync primitive that'd solve everything and wouldn't require SkiaSharp to include this platform specific stuff.
That said, anyone using SkiaSharp in an STA app is probably going to want the critical section solution (whether they realize it or not) and IMO is a better and safer default implementation for Windows.