Replace HandleDictionary lock with a critical section under Windows#1817
Conversation
| /// <summary> | ||
| /// Abstracts a platform dependant lock implementation | ||
| /// </summary> | ||
| interface IPlatformLock |
There was a problem hiding this comment.
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.
You still need some sort of locking because the HandleDictionary can be called from the GC thread.
There was a problem hiding this comment.
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.
| public NonAlertableWin32Lock () | ||
| { | ||
| _cs = Marshal.AllocHGlobal (Marshal.SizeOf<CRITICAL_SECTION> ()); | ||
| InitializeCriticalSectionEx (_cs, 4000, 0); |
There was a problem hiding this comment.
Should these calls check the result and throw using Marshal.GetLastWin32Error? At least we can catch an error and not break later on in a random place.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I mean the InitializeCriticalSectionEx may return false and have an error.
There was a problem hiding this comment.
Yes, I understand that... I just wasn't sure what exception to throw? Suggestions?
There was a problem hiding this comment.
Looking at people's code online, seems that they just use void and thus never care. I wonder why. Maybe it never fails?
There was a problem hiding this comment.
I looked in CoreRT and Sqlite. So I guess just ignore for now... I saw a comment on InitializeCriticalSection and InitializeCriticalSectionAndSpinCount so maybe it also applies:
This function always succeeds and returns a nonzero value.
Windows Server 2003 and Windows XP: If the function succeeds, the return value is nonzero. If the function fails, the return value is zero (0). To get extended error information, call GetLastError. Starting with Windows Vista, the InitializeCriticalSectionAndSpinCount function always succeeds, even in low memory situations.
There was a problem hiding this comment.
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.
mattleibow
left a comment
mattleibow
left a comment
There was a problem hiding this comment.
All right, not sure if you are still wanting to work on this after all this time, but I have been thinking about how I can have this as well as not add things to the global namespace as well as keep things neat and tidy.
I sort of decided to make this like - a bit inspired by discussions here on making this configuration point externally usable: #1856
So I think we can embrace this and maybe move the locking code into the internals namespace - probably can be in a separate file too.
We could have a public interface IPlatformLock and the have a static one-time configuration for now. Unless you feel inspired to have some way of making sure that we don't do something stupid and allow a lock system to be swapped out while things are mid lock.
public static class PlatformLock {
static Func<IPlatformLock>? _currentFactory;
public static void SetFactory (Func<IPlatformLock> factory) => _currentFactory = factory;
public static IPlatformLock Create() {
return _currentFactory?.Invoke() ?? new ReadWriteLock();
}
public class ReadWriteLock {}
public class NonAlertableWin32Lock {}
}And then usage from a library could be semi-discoverable:
PlatformLock.SetFactory(() => new PlatformLock.NonAlertableWin32Lock());or even generics...
PlatformLock.SetFactory<PlatformLock.NonAlertableWin32Lock>();Regardless, I think making it a feature and configurable will help for both your case and maybe Avalonia. Maybe there is some special platform lock on a unix machine that can do special things. Who knows.
|
Hi @mattleibow, I'm perfectly happy with whatever you think is the best solution - just so long as there's a way to get a non-alertable lock in there somehow. I think the biggest question is what lock it uses on Windows by default? Personally I think it should use the non-alertable lock because of reasons mentioned here. This decision doesn't really affect me because my toolkit will configure as appropriate but for general SkiaSharp users you need to make a call on that. I can make the changes if you let me know exactly how you want it to work (but probably won't get to it until Monday). If you prefer make the changes yourself I'm fine with that too. Brad |
|
Good point. Maybe we can change the default now in the previews and see what breaks. We can always change it back in a new preview. |
added ability to plugin custom factory
|
Hey Matt, I've updated the PR as follows:
So by default it uses non-alertable lock on Windows. If you want to change it... SkiaSharp.Internals.PlatformLock.Factory = () => new MyFunkyLock();Does that cover it? |
|
This is amazing. Thanks. Once the PR builds then it is ready to go! |
|
Just adding this for laters... I noticed this on the test run. Might be some bad code with the dictionary or lifetime things, but I will have a closer look after a nap: Looks like the GC is collecting things and the critical section is gone. Not sure why since it is static, so maybe it is the GC that has first collected the lock and is now collecting the object. Not sure if there is a way to tell the platform lock to be the last thing to be collected... Will have to see. Re-ran the tests to see if it passes or if this is reproducible. This was with the .NET Full Framework (.NET Core seemed fine). The GC is very different for the CoreCLR, Mono and netfx, so this is very possible. |
|
3 runs later and it is still the same error. I'll have a look and see if we are missing some lifetime thing. Hopefully a missing semicolon :) |
|
I can repro this with this command: If I remove the if statement and always use the slim lock, it works. It might be because the slim lock is a framework think and that outlives the tests/app. Time for some learning! |
|
I added some code to basically make every object keep a reference to the platform lock instance - to prevent the GC from collecting too soon. But, this seemed to throw another type of exception which seems to indicate that some object in the dictionary was not really that object. Maybe a pointer was replaced underneath - meaning that the locking did not protect that: |
|
Doing a bit more things and my logging shows that the lock is collected before all the objects are collected. No idea why since the objects invoke methods on the handle dictionary which reference the lock. Seems that the collection order is not correct. And the reason why there are no issues with .net core is because none of the static objects are collected. It probably has to do with AppDomains because I don't think .net core has them and the netfx unloads them at the end of all the tests - which triggers the GC. |
|
I see I do have a slight bad case where I load the statics from the C++ library and wrap them. I have code to avoid disposing them, but this is still incorrect as I just prevent the user disposing it. If the GC collects them and disposes of the native side, we have corrupted the native library. I fixed this by making sure I prevent all managed disposal since this is up to the native library. |
|
Just having a look at this and a less impactful fix might be to just remove the finalizer from |
|
It seems to have fixed itself now after I stopped collecting the things I should not be collecting. The old code was disposing of things that I did not create. And I think the fails only come up if I am doing it incorrectly. I'll rebuild and merge this if it is all green now. Then I will release a preview so we can test in the wild. If people get weird crashes, they can use the old lock and we can also change the default before we go stable. |
|
Thanks Matt, sounds good... I'm hoping to put up an alpha build of the new version of my app with this early next week so seems like the timing might work out nicely. |
|
Thanks for this, merging with happy thoughts. Hopefully all is well and we just fixed other folks' weird errors. |
|
Hey Matt, just wondering if there's preview build of this available yet somewhere? |
|
Yep, on the preview feed. https://aka.ms/skiasharp-eap/index.json |
|
I'm still seeing the |
Is this happening while the app is running or during process shutdown. My guess is the PlatformLock's finalizer has run before the last native object is collected and deleting the critical section here: SkiaSharp/binding/Binding/PlatformLock.cs Line 107 in e603133 Not sure what other reason EnterCriticalSection would crash. |
|
@toptensoftware I suspect you're right - it'll be when the AppPool is recycled. |
|
I merged this PR that basically turns a collected lock into a no-op and I assume all the objects will be collected in parallel - potentially: #2195 Is that worse? |
|
If it happens, it's possible that the Since the |
|
That'll should fix the crash, but I agree with @RichardD2 - it could cause the dictionary to become corrupt. Another safer but kludgier way to fix it might be to just never delete the critical section - it's a tiny blob of memory, there's only one instance. Just leave Windows to clean it up when the process exits. What I don't understand about this is there's methods being called on that lock instance after it's been GCed. Is that possible and/or a known edge case during shutdown? |
|
It may be calling the destruction in a random order. So first takes out the dictionary and then an object. But yeah, I agree for safety to not actually clear the critical section on the GC of the handle. Or maybe we should implement the disposal pattern properly :) But not sure if this makes a difference. |
3 participants
Description of Change
Fix for deadlock issue related to the lock implementation used by HandleDictionary under Windows. See issue #1383 for full description of issue.
IPlatformLockinterfaceIPlatformLockmaps to a Win32 Critical Section via PInvokeIPlatformLockmaps to aReaderWriterLockSlimas beforeTested on Windows 10 under a heavy load scenario where this issue was previously easily reproducible. After the fix issue couldn't be reproduced.
Bugs Fixed
-Related to issue: #1383
API Changes
None
Behavioral Changes