Make memory handling safe and comprehensible

[martinkosch]

Some variables can currently be created either in Julia or C memory space, e.g., UA_NODEID_NUMERIC(nsIndex::Integer, identifier::Integer)::UA_NodeId vs. UA_NodeId_new()::Ptr{UA_NodeId}
This makes using the package incomprehensible and prone to memory errors.

I can only think of solving this by allocating everything dynamically within the C memory space for now, i.e. use UA_..._new() for all variables and encourage using corresponding destructors. All other functions should then be removed or moved to another package with high-level functionality. Is there another way?

[thomvet]

I agree that we have to settle on one strategy here, but I am not 100% clear on which way to go.

What speaks against having a high level interface, for example: UA_NODEID_NUMERIC(nsIndex::Integer, identifier::Integer) and then below it doing the memory management in C? When would you have the destructors called?

I must admit, I don't have a lot of experience with memory management in C and I do find the garbage collector in Julia to be very convenient in general.

[martinkosch]

Same here :)
I also would like to keep all high-level functions as far as they are compatible with the one strategy. In this case, UA_NODEID_NUMERIC(nsIndex::Integer, identifier::Integer) should better move everything to the C memory space and should return a Ptr{UA_NodeId}.

However, those functions will make it hard to understand for users when to call a destructor, since the dynamic allocation is hidden within the function (actually, some of our current tests are also not clean in this respect). Therefore, I would prefer to use the following strategy:
Objects are only dynamically allocated when using a function UA_..._new() and must be accompanied by a corresponding destructor. The only issue in this case is that it is sometimes possible to call the destructors in the wrong order or twice on the same address, if I am not mistaken. What do you think?

Instead of high-level functions such as UA_NODEID_NUMERIC(nsIndex::Integer, identifier::Integer)::Ptr{UA_NodeId}, we could harmonize the nomenclature by overloading the UA_..._new() functions, e.g., UA_NodeId_new(nsIndex::Integer, identifier::Integer)::Ptr{UA_NodeId}.

[thomvet]

Instead of high-level functions such as UA_NODEID_NUMERIC(nsIndex::Integer, identifier::Integer)::Ptr{UA_NodeId}, we could harmonize the nomenclature by overloading the UA_..._new() functions, e.g., UA_NodeId_new(nsIndex::Integer, identifier::Integer)::Ptr{UA_NodeId}.

This could be quite nice from a usability standpoint. Take for example this code:

function UA_NodeId_new(nsIndex::Integer, identifier::Integer)
    nodeid = UA_NodeId_new()
    nodeid.namespaceIndex = UA_UInt16(nsIndex)
    
    identifier_tuple = open62541.anonymous_struct_tuple(UInt32(identifier),
        typeof(unsafe_load(nodeid.identifier)))
    nodeid.identifier = identifier_tuple
    return nodeid
end

And then we could have UA_NodeId_new(nsIndex::Integer, identifier::String) etc., i.e., using multiple dispatch.

(The way I have done it above, could also allow to revert one of the custom changes in generator.jl where the automatically generated struct for UA_NodeId is manually replaced... It felt like this is something that would be easily breaking in the future anyhow.)

What happens if I construct a server, add a variable to it, delete the nodeid with UA_NodeId_delete and then try to run the server? Does it run or not? Any implications of doing such deletions "too early" in general?

From a user perspective, it would of course be nice if that doesn't result in a crash.

[thomvet]

Made a test here. The server is startable. If I kill it (through multiple CTRL+C) and then try to start it again, it seems faulty. Don't understand that part yet.

https://github.com/thomvet/open62541.jl/blob/thomvet-typescleanup/src/memorytest.jl

[martinkosch]

That is really a good point. I would not know without checking the source code and we better don't expect the user to know :)

As far as I know, the fact that the server is startable is not even a clear indicator that the server makes its own copy of the UA_NodeID. It could instead only crash in certain situations when the memory is allocated for other purposes. But this is where my C knowledge really is limited. The only safe way to know is probably to follow the call stack, e.g., here and here.

When it comes to those questions, I hope there is a clear structure that open62541 is following. Maybe it is helpful to take a closer look at the corresponding source code and documentation. Unfortunately, I am currently relatively busy but will hopefully come back to this next week.

[thomvet]

The manual (https://www.open62541.org/doc/1.3/server.html?highlight=stack%20evaluates) says it makes a copy:

The methods for node addition and deletion take mostly const arguments that are not modified. When creating a node, a deep copy of the node identifier, node attributes, etc. is created. Therefore, it is possible to call for example UA_Server_addVariablenode with a value attribute (a Variant) pointing to a memory location on the stack. If you need changes to a variable value to manifest at a specific memory location, please use a Data Source Callback or a Value Callback.

(emphasis added is mine)

and the C-source code of UA_addVariableNode also agrees! So, that should be fine. Also, this kind of memory management (the "free"-ing part) only truly matters if repeated calls occur? So, it's critical for things like data reading and updating functions, but maybe less so for creating nodes (apart from avoiding crashes and unintended behavior of course)?

[thomvet]

I have had a little bit of a play with wrappers for NodeIds. Basically, I wanted to see whether there is good way of keeping the memory management on C-side and at the same time having Julia's garbage collector releasing the memory when there is no more reference to the respective pointer.

By getting inspired from other libraries (here: https://github.com/SciML/Sundials.jl/blob/efd1a0a04a99ed68b3bc5f42e94e8e2aecbfdf09/src/nvector_wrapper.jl and here: https://github.com/trixi-framework/P4est.jl/blob/main/src/pointerwrappers.jlfrom for example), I made some progress.

The result is here: https://github.com/martinkosch/open62541.jl/blob/wrapper-test/src/wrappers.jl
With some basic playing around here: https://github.com/martinkosch/open62541.jl/blob/wrapper-test/src/wrappertest.jl

Maybe there is a way of making this pretty neat in the end with an "out of the box" functionality using the low-level UA... API where one has to keep freeing memory in mind and a higher level one where things are getting taken care of in an easier fashion.

I'll keep exploring a little bit :)

[thomvet]

This progressed quite a bit with #18 and co. Even though the high level interface is not 100% complete yet, I think the pathway towards making things comprehensible is clear now. Just needs to be driven to completion, but I suggest to do that via more specific issues in the future.