Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implemented RCL global argument parsing. #122

Merged
merged 5 commits into from
Oct 16, 2023
Merged

Implemented RCL global argument parsing. #122

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
30b2f06
Integrating RCL global argument parsing.
alectutin-qut Sep 22, 2023
d3122ca
Integrating retrieval of node name and namespace.
alectutin-qut Sep 22, 2023
232a4e0
Merge pull request #1 from Chootin/tutina/parsing_args
Chootin Sep 22, 2023
7d3c0a3
Implementing feedback from @hoffmann-stefan
alectutin-qut Oct 12, 2023
c51b096
Removing use of generic.
alectutin-qut Oct 16, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 24 additions & 0 deletions rcldotnet/Node.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,14 @@ internal delegate RCLRet NativeRCLActionDestroyServerHandleType(

internal static NativeRCLActionDestroyServerHandleType native_rcl_action_destroy_server_handle = null;

[UnmanagedFunctionPointer(CallingConvention.Cdecl)]
internal delegate string NativeRCLGetStringType(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Returning a string directly from an PInvoke is a new pattern that isn't used anywhere yet in this code base.
After some research this seems problematic:

from https://www.mono-project.com/docs/advanced/pinvoke/

It’s easy to skim over memory management for most of Platform Invoke and marshaling, but for return values the CLI implements some default handling which must be considered.

The CLI runtime assumes that, under certain circumstances, the CLI runtime is responsible for freeing memory allocated by unmanaged code. Return values are one of those circumstances, causing the return value to be a memory boundary for control of memory (de)allocation.

The CLI assumes that all memory that is passed between the CLI/unmanaged code boundary is allocated via a common memory allocator. The developer does not get a choice in which memory allocator is used. For managed code, the Marshal.AllocCoTaskMem method can be used to allocate memory, Marshal.FreeCoTaskMem is used to free the memory allocated by Marshal.AllocCoTaskMem, and Marshal.ReAllocCoTaskMem is used to resize a memory region originally allocated by Marshal.AllocCoTaskMem.

Since classes are passed by reference, a pointer is returned, and the runtime assumes that it must free this memory to avoid a memory leak. The chain of events is thus:

  1. Managed code invokes unmanaged function that returns a pointer to an unmanaged structure in unmanaged memory.
  2. An instance of the appropriate managed class is instantiated, and the contents of the unmanaged memory is marshaled into the managed class.
  3. The unmanaged memory is freed by the runtime “as if” by invoking Marshal.FreeCoTaskMem().

How is Marshal.AllocCoTaskMem, Marshal.ReAllocCoTaskMem, and Marshal.FreeCoTaskMem implemented? That’s platform-dependent. (So much for portable platform-dependent code.) Under Windows, the COM Task Memory allocator is used (via CoTaskMemAlloc(), CoTaskMemReAlloc(), and CoTaskMemFree()). Under Unix, the GLib memory functions g_malloc(), g_realloc(), and g_free() functions are used. Typically, these correspond to the ANSI C functions malloc(3), realloc(3), and free(3), but this is not necessarily the case as GLib can use different memory allocators; see g_mem_set_vtable() and g_mem_is_system_malloc() .

What do you do if you don’t want the runtime to free the memory? Don’t return a class. Instead, return an IntPtr (the moral equivalent of a C void* pointer), and then use the Marshal class methods to manipulate that pointer, such as Marshal.PtrToStructure, which works for both C# struct types and class types marked [StructLayout(LayoutKind.Sequential)].

My understanding of this is the following:
While this might work for linux as the runtime and the get_str_cpy use the same allocator, this will most certainly break on windows as then CoTaskMemFree() will get called on the pointer allocated by malloc().

So I would propose to use the method mentioned from the mono documentation and return an IntPtr instead and use Marshal.PtrToStringAnsi()
This method is used in the marshalling of the ROS messages as well.

Normally it should be fine to access the returned pointer directly after the call and pass it to Marshal.PtrToStringAnsi() as the livetime of the returned char* is documented to be valid as longe as the rcl_node_t is valid:

const char * native_rcl_get_name_handle(void *node_handle) {
  rcl_node_t *node = (rcl_node_t *)node_handle;

  // remove get_str_cpy
  return rcl_node_get_name(node);
}
public string GetName()
{
    IntPtr namePtr = NodeDelegates.native_rcl_get_name_handle(Handle);
    return Marshal.PtrToStringAnsi(namePtr);
}

But I think we should add additional SafeHandle.DangerousAddRef()/SafeHandle.DangerousRelease() around this to stay safe when some other thread does Dispose() the SafeNodeHandle (disposing nodes isn't implemented right now, but if we don't add this right now we will forget and only add it after a long memory management debugging session ;))

public string GetName()
{
    bool mustRelease = false;
    try
    {
        // This avoids accessing a invalid/freed pointer if some other thread disposes the SafeNodeHandle.
        Handle.DangerousAddRef(ref mustRelease);
        IntPtr namePtr = NodeDelegates.native_rcl_get_name_handle(Handle);
        return Marshal.PtrToStringAnsi(namePtr);
    }
    finally
    {
        if (mustRelease)
        {
            Handle.DangerousRelease();
        }
    }
}

But it's getting late here, so maybe I need to think about this again after some sleep :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very insightful analysis. I had noticed the behaviour of freeing the memory from the managed side and assumed that simply returning a copy of the string was the best way to handle that at the time. I'll have to put some time into considering how this affects other code I have written after this initial pull request!

SafeNodeHandle nodeHandle);

internal static NativeRCLGetStringType native_rcl_get_name_handle = null;

internal static NativeRCLGetStringType native_rcl_get_namespace_handle = null;

static NodeDelegates()
{
_dllLoadUtils = DllLoadUtilsFactory.GetDllLoadUtils();
Expand Down Expand Up @@ -182,6 +190,18 @@ static NodeDelegates()
NodeDelegates.native_rcl_action_destroy_server_handle =
(NativeRCLActionDestroyServerHandleType)Marshal.GetDelegateForFunctionPointer(
native_rcl_action_destroy_server_handle_ptr, typeof(NativeRCLActionDestroyServerHandleType));

IntPtr native_rcl_get_name_handle_ptr = _dllLoadUtils.GetProcAddress(
nativeLibrary, "native_rcl_get_name_handle");
NodeDelegates.native_rcl_get_name_handle =
(NativeRCLGetStringType)Marshal.GetDelegateForFunctionPointer(
native_rcl_get_name_handle_ptr, typeof(NativeRCLGetStringType));

IntPtr native_rcl_get_namespace_handle_ptr = _dllLoadUtils.GetProcAddress(
nativeLibrary, "native_rcl_get_namespace_handle");
NodeDelegates.native_rcl_get_namespace_handle =
(NativeRCLGetStringType)Marshal.GetDelegateForFunctionPointer(
native_rcl_get_namespace_handle_ptr, typeof(NativeRCLGetStringType));
}
}

Expand Down Expand Up @@ -440,5 +460,9 @@ private static CancelResponse DefaultCancelCallback(ActionServerGoalHandle goalH
{
return CancelResponse.Reject;
}

public string GetName() => NodeDelegates.native_rcl_get_name_handle(Handle);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should use a get-only-computed property here instead of getter methods, but I can see arguments for using a method here as well. Have you considered properties instead?

  public string Name => NodeDelegates.native_rcl_get_name_handle(Handle);
  public string Namespace => NodeDelegates.native_rcl_get_namespace_handle(Handle);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. I think I will also include a FullyQualifiedNodeName in the same way as it is useful for the parameter handling code.


public string GetNamespace() => NodeDelegates.native_rcl_get_namespace_handle(Handle);
}
}
6 changes: 4 additions & 2 deletions rcldotnet/RCLdotnet.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,8 @@ internal static class RCLdotnetDelegates
internal static readonly DllLoadUtils _dllLoadUtils;

[UnmanagedFunctionPointer(CallingConvention.Cdecl)]
internal delegate RCLRet NativeRCLInitType();
internal delegate RCLRet NativeRCLInitType(
int argc, [MarshalAs(UnmanagedType.LPArray, ArraySubType = UnmanagedType.LPStr)] string[] argv);

internal static NativeRCLInitType native_rcl_init = null;

Expand Down Expand Up @@ -1396,7 +1397,8 @@ public static void Init()
{
if (!initialized)
{
RCLRet ret = RCLdotnetDelegates.native_rcl_init();
string[] args = System.Environment.GetCommandLineArgs();
RCLRet ret = RCLdotnetDelegates.native_rcl_init(args.Length, args);
RCLExceptionHelper.CheckReturnValue(ret, $"{nameof(RCLdotnetDelegates.native_rcl_init)}() failed.");
initialized = true;
}
Expand Down
7 changes: 2 additions & 5 deletions rcldotnet/rcldotnet.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,18 +28,15 @@
static rcl_context_t context;
static rcl_clock_t clock;

int32_t native_rcl_init() {
// TODO(esteve): parse args
int num_args = 0;
int32_t native_rcl_init(int argc, const char *argv[]) {
context = rcl_get_zero_initialized_context();
rcl_allocator_t allocator = rcl_get_default_allocator();
rcl_init_options_t init_options = rcl_get_zero_initialized_init_options();
rcl_ret_t ret = rcl_init_options_init(&init_options, allocator);
if (RCL_RET_OK != ret) {
return ret;
}
const char ** arg_values = NULL;
ret = rcl_init(num_args, arg_values, &init_options, &context);
ret = rcl_init(argc, argv, &init_options, &context);
if (ret != RCL_RET_OK) {
return ret;
}
Expand Down
2 changes: 1 addition & 1 deletion rcldotnet/rcldotnet.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
#include "rcldotnet_macros.h"

RCLDOTNET_EXPORT
int32_t RCLDOTNET_CDECL native_rcl_init();
int32_t RCLDOTNET_CDECL native_rcl_init(int argc, const char *argv[]);

rcl_clock_t *native_rcl_get_default_clock();

Expand Down
22 changes: 22 additions & 0 deletions rcldotnet/rcldotnet_node.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -240,3 +240,25 @@ int32_t native_rcl_action_destroy_server_handle(void *action_server_handle, void

return ret;
}

// Avoid problems caused by automatic free of the original string.
const char * get_str_cpy(const char * src) {
size_t size = strlen(src) + 1;

char *copy = (char *)malloc(size);
memcpy(copy, src, size);

return copy;
}

const char * native_rcl_get_name_handle(void *node_handle) {
rcl_node_t *node = (rcl_node_t *)node_handle;

return get_str_cpy(rcl_node_get_name(node));
}

const char * native_rcl_get_namespace_handle(void *node_handle) {
rcl_node_t *node = (rcl_node_t *)node_handle;

return get_str_cpy(rcl_node_get_namespace(node));
}
6 changes: 6 additions & 0 deletions rcldotnet/rcldotnet_node.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,4 +71,10 @@ int32_t RCLDOTNET_CDECL native_rcl_action_create_server_handle(void **action_ser
RCLDOTNET_EXPORT
int32_t RCLDOTNET_CDECL native_rcl_action_destroy_server_handle(void *action_server_handle, void *node_handle);

RCLDOTNET_EXPORT
const char * RCLDOTNET_CDECL native_rcl_get_name_handle(void *node_handle);

RCLDOTNET_EXPORT
const char * RCLDOTNET_CDECL native_rcl_get_namespace_handle(void *node_handle);

#endif // RCLDOTNET_NODE_H