search

TwinFan / XPMP2

Multiplayer library for X-Plane 11/12 with instancing, TCAS override, and sound
https://twinfan.github.io/XPMP2/
Other
24 stars 16 forks source link

Fix data race during Instance Deletion. #33

Closed mikeyusc closed 3 years ago

mikeyusc commented 3 years ago

So I could be totally off base here.. but it looks like in Aircraft::DestroyInstances(), you call XPLMDestroyInstance, then you remove it from the list in lines 618 & 619.. assuming DoMove is being called on the aircraft in another thread at the same time, timed exactly between these two lines, it's possible for you to end up calling XPLMInstanceSetPosition on line 505, with an invalid instance handle, which if xPlane is using as a pointer, would explain the bogus issues we're seeing.. it's not the data pointers that are invalid, it's the instance #23

TwinFan commented 3 years ago

Thanks for thinking about our problem. I could agree in a multi-threaded world, but X-Plane is - at its core - single theaded and so is the code:

Aircraft::DestroyInstance is secured against execution in a worker thread, see lines 610-614. This is because X-Plane does not allow calling any instance manipulating function from a worker thread (ie. a thread which is not the X-Plane main thread). X-Plane would creash with the THREAD FATAL ASSERT message. And because it actually did right here, introduced in some XP11.50 beta release, I had to implement the delaying solution via Aircraft::bDestroyInstance because I wanted to support calling Aircraft::SetInvalid from a worker thread. Aircraft::bDestroyInstance is checked only later in a main thread call. This is in Aircraft::FlightLoopCB, line 429, from where afterwards also DoMove is called - after the instance is safely removed. So there cannot be two threads at the same time invoking XPLM...Instance calls as at least one of them must be in a worker thread, which isn't allowed. Or in other words: All XPLM...Instance calls are guaranteed by X-Plane to happen single-threaded. And so is the removal from listInst as it is guarded against worker thread execution, too.

Also, we do not see "Instance destroyed" entries in Log.txt in crashing instances (as would be written by line 621 a moment later), but in contrary we see "instance created" entries right before a crash.

You code change may probably not be harmful, it might in fact even be good coding style to prevent bad things to happen in a potentially multi-threaded world, but it won't solve #23 unfortunately.

mikeyusc commented 3 years ago

I've been working with Justin on support with Xpilot, and virtually every crash that users are reporting that seems to be related to this issue has a DestroyInstance call within less than a second before the crash.

TwinFan commented 3 years ago

You are aware of my tests with millions of plane creations and destructions, which did not crash the sim?

But hey...as said...the code change will do no harm and is good coding style, so I'll merge it. Prove me wrong...would be great...I'd also like to get rid of #23. Just setting expectations...