search

sharpdx / SharpDX

SharpDX GitHub Repository
http://sharpdx.org
MIT License
1.69k stars 641 forks source link

Shadow initialization isn't thread-safe. #1019

Open Jayonas opened 6 years ago

Jayonas commented 6 years ago

I had an issue where I was occasionally getting an E_INVALIDARG exception from my call to TextLayout.Draw, and it happened more frequently the more Draw calls were happening around the same time, pointing to some kind of multi-threading style issue. It turned out that DWrite was receiving nullptr for the IDWriteTextRenderer parameter to IDWriteTextLayout::Draw, and I verified that I always passed a non-null managed TextRenderer instance.

Looks like the problem is in CppObject.ToCallbackPtr where an ICallbackable's Shadow is created/initialized if necessary, because the check/creation/initialization isn't thread-safe. I'm pretty sure what happened was that thread A saw that callback.Shadow was null and proceeded to create a ShadowContainer and call Initialize on it. The first thing Initialize does is assign the new container to the callback's Shadow, making it non-null, and before Initialize could finish (might easily block the thread due to acquiring a lock shortly thereafter) thread B came along and saw that the callback's Shadow was non-null, so proceeded to call Find on it. Since it hadn't actually been initialized yet no native callback pointer was available so Find returned null, and thread B passed that null on down to DWrite.

I was able to work around the problem by just manually calling CppObject.ToCallbackPtr in the constructor of any class that implements ICallbackable (e.g. my TextRenderer implementation). I just made a helper base type that inherits from CallbackBase and does that call in its constructor, and all my callback types derive from that instead of CallbackBase. That call ensures that each object's Shadow is initialized as part of its construction process, so by the time multiple threads have their hands on the object and are trying to use it, they won't be racing to be the first one to initialize the Shadow.

(Note that I'm basing this off my usage of SharpDX 3.1.1, which I realize is out of date, but I diagnosed the issue based on the latest SharpDX code.)

xoofx commented 6 years ago

Yes, I think that I didn't expect it to be thread safe to avoid costly lock for non multi-threading scenarios... but I think that's ok to add a lock in ToCallbackPtr, PR welcome.

Jayonas commented 5 years ago

I've recently updated SharpDX, and I can confirm that this issue still exists in 4.2.0.