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

Reference counter >0 during shutdown of long sessions #18

Closed TwinFan closed 4 years ago

TwinFan commented 4 years ago

Describe the bug After a long (2.5h) LiveTraffic session with lots of traffic, a couple of reference counters don't reach 0 during shutdown. The cars combined even stay at 8:

2:30:32.287    LT/XPMP2 DEBUG CSLModels.cpp:197/Unload: Object __XCSL_CARS/CARS_FollowMe / Resources/plugins/LiveTraffic/Resources/ShippedCSL/XCSL_CARS/ZZZC2.obj unloaded
2:30:32.287    LT/XPMP2 WARN  CSLModels.cpp:348/~CSLModel: Reference counter is 3 while object is being destroyed for __XCSL_CARS/CARS_FollowMe
2:30:32.287    LT/XPMP2 DEBUG CSLModels.cpp:197/Unload: Object CARS/CARS_ZZZC / Resources/plugins/LiveTraffic/Resources/CSL/X-CSL/CARS/ZZZC2_ZZZC2.obj unloaded
2:30:32.287    LT/XPMP2 WARN  CSLModels.cpp:348/~CSLModel: Reference counter is 5 while object is being destroyed for CARS/CARS_ZZZC

To Reproduce / Source Code Example One way to make it very likely to happen is in LiveTraffic: Re-load an already loaded CSL package. That will ask all flying planes to re-assess their models, but as no actually new models are available many planes will pick the very same model once again, which causes the invalid reference counter handling, see below.

Expected behavior No such messages, reference counter should reach zero in all cases.

Version

Log.txt Log_RefCnt.txt.zip

Additional context Discussed on Discord, but I have seen such messages in xPilot logs before but had at that time attributed to wrong implementation there. Unclear, as I don't know their implementation.

TwinFan commented 4 years ago

Potential leak: Both Aircraft::ChangeModel() and Aircraft::AssignModel() have a path that could lead to calling pCSLMdl->IncRefCnt() without a matching call to pCSLMdl->DecRefCnt():

When the determined "new" model pMdl is the same as the existing model pCSLMdl then the code to remove the old model is skipped, but the not-so-new one is stored nonetheless. That is safe coding with regard to pointer and derived information, but calling IncRefCnt() is then wrong.

TwinFan commented 4 years ago

ChangeModel() could be called at any time, in LiveTraffic for example when new aircraft master data arrives that changes model-defining fields. That happens, though it is comparibly rare a case.

However, in the log attached above there was one case of loading additional CSL models during runtime (at timestamp 0:46:37.231). in this case, LiveTraffic sets all flying planes to re-evaluate models. That particular loading of models was rather unsuccessful (many discarded as duplicates), so many planes will eventually pick the very same model again...which is exactly the trigger for the wrong handling of the reference counter above.

So to reproduce the scenario the easiest was is to re-load an already loaded CSL package.