search

logmanoriginal / lvssh2

LabVIEW­ bindings for libssh2
BSD 3-Clause "New" or "Revised" License
0 stars 3 forks source link

lvssh_extensions Multi-thread Safety and Memory Allocation #3

Open j-medland opened 6 months ago

j-medland commented 6 months ago

Hello - I have done quite a bit with C/C++ integration into LabVIEW and I am always interested to see what other folks are doing.

Thanks for contributing to the community and please don't feel obliged to respond to this issue. I am a firm believer that if you write code that solves your problem then that's great and if you share it with the community, even better. You don't owe anything else.

Now - my questions

Question 1 I was curious to know how you are managing multi-thread safety in your lvssh_extension code:

It looks like you declare some global variables to store pointers to memory and it appears from the LV-code that you could have multiple open sessions running which would all be accessing the same global variables from potentially (depending on the LabVIEW thread-managment) different threads.

Do you manage or protect against race conditions in some way (from the C-code side or the LabVIEW side)?

Question 2 You seem to be allocating onto the heap (with malloc) in a number of function calls with objects that are then passed to LVPostUserEvent.

Is there a particular reason for this - for situations where the allocated memory shares the scope of the function (i.e. you call free toward the end of the function), I think stack allocation is fine. For variables that are basic data types you can just decalre and intialize them and then use & to get their address which you pass to LVPostUserEvent.

Question 3 I have a rough CMake configuration that will handle building and installing the shared-library files for 32 and 64 bit Windows and 64-bit Linux if you are interested. It requires some modifications to the .h and .cpp files as you are using some Windows specific conventions and syntax. It is also capable of building libssh2 as a dependency and locating LabVIEW's cintools on those platforms.

Would you be interested in a PR with it in? This would be in addition to my current PR #2 and would also require you to undertake the testing as I don't quite know what would need to be done to ensure it works on the platforms specified.

Cheers

logmanoriginal commented 6 months ago

Thanks for taking your time to review the code! I'm always happy to receive feedback and suggestions for improvement.

As a side note, I typically work with LV or C#. Consequently, my C/C++ skills are very basic. Turns out reading C++ is MUCH easier than writing it 😭

Question 1: Thread-safety This is a very good point. The extension library is not thread-safe and there are no precautions in the LabVIEW code. I have considered passing an object to the callback VI (for the return values) instead of using global variables but couldn't figure out a good way to make it work without getting lost in pointer hell on the LabVIEW side. This is one of the reasons why the extension library can globally be disabled; it is quite fragile.

Question 2: Heap allocation There is no particular reason for this, other than being the only way I could figure out how to make it work. This might also explain the issues I saw after reviewing #2 regarding the heap corruption exceptions.

Question 3: CMake I'd be happy to and can take care of testing. At least the Windows side as I currently don't have a functional Linux machine with LabVIEW installed.