JKISoftware / JKI-State-Machine

JKI State Machine
Other
42 stars 21 forks source link

Editor Plug-ins that call "Term:Data Type" (as Variant) need a Request Deallocation to avoid LV crash #10

Closed AristosQueue closed 5 years ago

AristosQueue commented 5 years ago

On behalf of LabVIEW, let me apologize for this bug report. The JKI State Machine right-click plug-in is crashing some people's LabVIEWs, and that's something NI ought to be able to fix, but we cannot, so it falls to the G programmers to compensate for our shortcomings. Ok... my shortcomings.

This VI is part of your right-click plug-in: C:\Program Files\National Instruments\LabVIEW 2018\resource\dialog\QuickDrop\plugins_JKI\State Machine Helper.llb\Is String Data Source Terminal Connected to Add States SubVI__JKI_State_Machine_Helper.vi

It calls properties/methods that copy data out of the user's VI into the plug-in VI in variants. Specifically, "Term:Data Type". That data can include LabVIEW classes. Because of a bug in LabVIEW, LabVIEW cannot properly account for data of class X that is copied out of the application instance. That was, in fact, never supposed to be legal under the design of LV classes, but someone had the bright idea to use Variants to represent type, and those carry data. That means that any one who calls anything that gets data type that calls across application instances must either leave memory before that class does (so the data gets deallocated while the class is still in memory) OR must call Request Deallocation so that it frees all the copies of the class data that are inside itself on every call.

Please add a Request Deallocation node to the top-level diagram of this VI (and any other plug-in VI that calls a Data Type function) to fix this crash.

You may also need to do that to the the "Get TDEnum From Data blah blah blah" VI that takes the variant as an input, but probably not if the subVI doesn't store the variant.

I am sorry for this. I wish there were a straightforward way for LV to address this weakness. I haven't come up with one.

jimkring commented 5 years ago

Thanks for reporting this issue and fix, @AristosQueue. We've had reports of odd, occasional LabVIEW crashing over the period of using the editor, but haven't been able to pin down the cause. Maybe it's related.

Also, I haven't used the "Request Deallocation" function before. Do we need to pass a TRUE to the flag argument? Does this function no-op if a FALSE is passed in? I tried to read the documentation, but it's not totally clear to me about the purpose of this flag.

image

jimkring commented 5 years ago

Also, @AristosQueue, I found that the "Term:Data Type" property is called in a few locations of the code. We'll want to do a Request Deallocation in all those places, too, right? And, maybe we can avoid passing the Variant into SubVIs (e.g. Get TDEnum From Data) by simply testing the data type with a Variant to Data function call (and testing for No Error out). Since Get TDEnum From Data is an OpenG function, I'd prefer not to modify it.

image

AristosQueue commented 5 years ago

@jimkring Yes, wire an unconditional TRUE to the primitive. Technically, you only need to call this primitive if a property that returns a variant for another context is called, but I typically put it on the top-level block diagram.

I found that the "Term:Data Type" property is called in a few locations of the code. We'll want to do a Request Deallocation in all those places, too, right?

You only need to do it if the reference is across application instances. If this is part of the plug-ins, then, yes.

AristosQueue commented 5 years ago

If you pass TRUE to the Request Deallocation node, it sets a flag on the calling VI. That's all it does. When the VI finishes its current execution, if that flag is set, the VI deflates its data space -- wipes out the operate value of controls/indicators, clears away the temporary space. Normally LV leaves those allocated because we get much better performance on second call (see my presentation on memory usage at the Americas CLA Summit in 2017 for details). The Request Deallocation is normally used for spiky VIs -- VIs that are called infrequently with very large data sets and you want to free that temporary data. But in this case, it's used to get all of the variants to get rid of any contained LV class data that was copied from the other application instance.

jimkring commented 5 years ago

Got it. Thanks, @AristosQueue!

jimkring commented 5 years ago

Fixed in Release 2018.0.2.37 and available for download/installation now in VIPM.

AristosQueue commented 5 years ago

For the record, the easiest way to test for bugs like these is as folllows:

  1. Create a new project.
  2. Look at your code that reads Data Type properties... if it is a signal's type, then create a VI that has a LV class (other than LabVIEW Object) signal. If it is a control then create a LV class control, etc. Right click on the thing such that your code reads the data type. Do this even if your right-click plug-in doesn't operate on classes!
  3. Having done the right click, close the project.
  4. Without quitting LV, open a VI that has another thing that your plug-in reacts to and right-click on it. If that causes LV to crash, you almost certainly have encountered this bug which can be solved by adding a Request Deallocation primitive.

I continue to explore ways to fix this bug that don't cause problems for the rest of the system. In the meantime, I'm glad you were able to fix this quickly, and I hope this message helps you avoid the issues in the future.

jimkring commented 5 years ago

Thanks for that info @AristosQueue. Thanks for keeping The LabVIEW awesome!