6
\$\begingroup\$

I want to use an existing C++ library in a C# app. The C++ library will have async callbacks for things like OnConnect and user updates so I want to reflect this in the eventual C# code. I would appreciate thoughts on the following implementation particularly in the area of memory leaks in the CLI part.

To begin, I have a simple C++ class:

#include <string>

class CppClass
{
public:

    typedef void(*Callback)(int);

    CppClass(){};
    ~CppClass(){};

    void Connect(std::string conn_info)
    {
        OnConnect();
    };

    void OnConnect()
    {
        m_callback(42);
    };

    void SetOnCallback(Callback callback)
    {
        m_callback = callback;
    };

private:
    Callback m_callback;
};

To communicate with this, I have a C++/CLI wrapper:

using namespace System;
using namespace System::Runtime::InteropServices;

namespace CliCpp {

    public delegate void OnConnectDelegate(int);

    public ref class WrapperClass
    {
    public: 

        // C# callback delegates
        CliCpp::OnConnectDelegate^ OnConnectHandler;

        // Ctor / dtor
        WrapperClass()
        {
            m_native = new CppClass();

            // Set OnConnect callback
            CliCpp::OnConnectDelegate^ managed_on_connect = gcnew CliCpp::OnConnectDelegate(this, &WrapperClass::OnConnect);
            IntPtr stub_ptr = Marshal::GetFunctionPointerForDelegate(managed_on_connect);
            CppClass::Callback fptr = static_cast<CppClass::Callback>(stub_ptr.ToPointer());
            m_native->SetOnCallback(fptr);
            GC::KeepAlive(managed_on_connect);
        };

        ~WrapperClass()
        {
            this->!WrapperClass();
        };

        !WrapperClass()
        {
            if (m_native != nullptr)
                delete m_native;
        };

        // Methods
        void Connect(String^ conn_info)
        {
            m_native->Connect(msclr::interop::marshal_as<std::string>(conn_info));
        };

        // Event handler from C++ code
        void OnConnect(int i)
        {
            // Call C# code:
            OnConnectHandler(i);
        };      

    private:
        CppClass* m_native;     
    };
}

And finally, I have some C# calling code:

using CliCpp;

namespace Caller
{
    class Program
    {   
        static void Main(string[] args)
        {
            using (WrapperClass cls = new WrapperClass())
            {
                // Set up the C# callback
                cls.on_connect_handler = OnConnect;
                cls.Connect("Hello");
                Console.ReadLine();
            }
        }

        // Callback Function
        static void OnConnect(int i)
        {
            Console.WriteLine("C# handler called!");
        }

    }
}

So, what I would like an opinion on is:

  1. Is this implementation of callbacks good practice?
  2. In the C++/CLI constructor where I set up the callback, am I using gcnew and 'GC::KeepAlive(x)' correctly so as not to leak (do I need a corresponding delete etc.)?

Any input appreciated!

\$\endgroup\$
  • \$\begingroup\$ Mixing three different languages. What can go wrong :-) \$\endgroup\$ – Martin York Aug 5 '16 at 16:23
  • \$\begingroup\$ Maybe it would be easier to implement the non-async methods on the wrapper, I don't know much about callbacks and this stuff though. \$\endgroup\$ – null Aug 5 '16 at 19:48
  • 1
    \$\begingroup\$ As far as I can see, this is how interop between C++ and .Net is encouraged.I don't think sync calls should make any difference since calls will originate either side of the CLI bridge in any case. \$\endgroup\$ – Pat Mustard Aug 8 '16 at 1:19
3
\$\begingroup\$

I'm not sure about the callbacks but you are using the destructor/finalizer in a way that might cause problems.

   ~WrapperClass()
    {
        this->!WrapperClass();
    };

    !WrapperClass()
    {
        if (m_native != nullptr)
            delete m_native;
    };

If you are doing this then the descructor should call GC.SuppressFinalize(this); because the finalizer has already been called.

Then in your finalizer you should set the m_native = nullptr; because if the GC calls it again later it will crash as the object has been deleted but it's still pointing to it.

    !WrapperClass()
    {
        if (m_native != nullptr)
        {
            delete m_native;
            m_native = nullptr;
        }
    }

As to the m_native I'd call it _nativePtr.

\$\endgroup\$
  • \$\begingroup\$ Would it make sense to just have a Destructor and leave out the finalizer? \$\endgroup\$ – Pat Mustard Aug 9 '16 at 1:32
  • \$\begingroup\$ @PatMustard You need both. The finalizer should clean-up unmanaged objects and the descructor managed ones but it can call the finalizer. The need for C++/CLI finalize destructor \$\endgroup\$ – t3chb0t Aug 9 '16 at 3:50
  • \$\begingroup\$ Understood - thx! \$\endgroup\$ – Pat Mustard Aug 9 '16 at 3:58

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.