6
\$\begingroup\$

The code appears to work fine. The values are returned as expected from the DLL and the values are added correctly to the DLL.

Will the way I am passing the string to C# cause a memory leak?

Also, I have to return a string from C# as type LPSTR to add to a char pointer, how could I change it so I could use the string type for the return value.

Also, I assume its related to the .Net garbage collector not kicking in yet but here is the values in TaskManager at different points:

First Line Of C Code          =  1,424k
Just After Create Instance    =  6,752k
Just Before Calling EndString =  26,264k
After EndString               =  26,276k
After CoUninit                =  26,320k

I would have expected the values to drop back down? Or do we have to wait for the garbage collector?

Here is the sample code I put together in Visual Studio 6.0:

#include "stdafx.h"
#include <string>
using namespace std;

#import "C:\\Documents and Settings\\Administrator\\My Documents\\Visual Studio 2008\\Projects\\COMCallable\\COMCallable\\bin\\Release\\COMCallable.tlb" raw_interfaces_only
using namespace COMCallable;

MyNetInterfacePtr pComObject;

string RetString();

void AddValue(long i);

void AddValue2(long i);

int main(int argc, char* argv[])
{
    HRESULT hr = S_OK;

    hr = CoInitialize(NULL);

    if(pComObject == NULL)
    {       
        pComObject.CreateInstance(__uuidof(MyNETClass));
    }

    pComObject->StartString();

    for(long i = 1; i <= 100000; i++)
    {
        printf("Value %ld: %s\r\n", i, RetString().c_str()); 

        AddValue(i);
    }

    pComObject->EndString();

    pComObject->Release();

    CoUninitialize();

    while(true)
    {
        Sleep(1000);
    }

    return 0;
}

string RetString()
{
    char *c = NULL;

    if(pComObject != NULL)
    {
        pComObject->TestMethodString(&c);
    }

    if(c != NULL)
    {
        return c;
    }
    else
    {
        return "";
    }
}

void AddValue(long i)
{
    char numstr[21] = {0}; // enough to hold all numbers up to 64-bits
    sprintf(numstr, "%ld", i);

    string s = string("Some String Here ") + string(numstr);

    if(pComObject != NULL)
    {
        pComObject->AddString((char *)s.c_str());
    }
}

And here is the .NET code:

Interface:

using System;
using System.Collections.Generic;
using System.Text;
using System.Runtime.InteropServices;

namespace COMCallable
{
    [ComVisible(true)]
    public interface MyNetInterface
    {

        [return: MarshalAs(UnmanagedType.LPStr)]
        string TestMethodString();

        void StartString();

        void AddString([MarshalAs(UnmanagedType.LPStr)]string s);

        void EndString();
    }
}

Class

using System;
using System.Collections.Generic;
using System.Text;
using System.Runtime.InteropServices;
using System.Threading;
using System.Diagnostics;

namespace COMCallable
{
    [ComVisible(true)]
    public class MyNETClass : MyNetInterface
    {
        private StringBuilder sb = new StringBuilder();

        public string TestMethodString()
        {
            return "Some String ";
        }

        public void StartString()
        {
            sb = null;
            sb = new StringBuilder();
        }

        public void AddString(string s)
        {
            if (sb == null)
            {
                sb = new StringBuilder();
            }

            sb.Append(s);
        }

        public void EndString()
        {
            sb = null;
        }
    }
}
\$\endgroup\$
3
\$\begingroup\$

I would have expected the values to drop back down? Or do we have to wait for the garbage collector?

That's not how the GC works. AFAIK, it only returns memory to the OS if the OS says it'as low on memory. The assumption is that if the OS doesn't need the memory, then it's better to keep the memory as a part of the managed heap, so that the GC doesn't need to run that often.


using namespace std;

This is generally considered a bad practice in C++, since the std namespace contains many simple names that you might want to use yourself.


#import "C:\\Documents and Settings\\Administrator\\My Documents\\Visual Studio 2008\\Projects\\COMCallable\\COMCallable\\bin\\Release\\COMCallable.tlb" raw_interfaces_only

I think that both projects should be part of the same solution and in under the same parent directory. That way, this path can be relative, which will make it much short and it will also work if you copy the whole solution to another computer.


MyNetInterfacePtr pComObject;

I think this shouldn't be a global variable. If you need to pass the object from one function to another, use parameters.


HRESULT hr = S_OK;

hr = CoInitialize(NULL);

There is no reason to initialize hr if you're going to overwrite the value right away anyway.

And if you know that a function can fail, you should check whether it failed or not, not just ignore the HRESULT.


if(pComObject == NULL)

pComObject has to be NULL at this point, so there is no reason for this check.


while(true)
{
    Sleep(1000);
}

If you want to wait forever, the documentation says you should be able to use Sleep(INIFINITE);.


char *c = NULL;

if(pComObject != NULL)
{
    pComObject->TestMethodString(&c);
}

I don't know much about memory management in COM, but this certainly looks like a memory leak. pComObject can't know when are you done using it, so it can't deallocate it itself and you also don't deallocate it.


sprintf(numstr, "%ld", i);

The documentation says:

Using sprintf, there is no way to limit the number of characters written, which means that code using sprintf is susceptible to buffer overruns.

By my count, 21 characters should be enough here, but I would probably use the safer version anyway, just to be sure.


if(pComObject != NULL)
{
    pComObject->AddString((char *)s.c_str());
}

So, if pComObject is NULL, this function doesn't do anything? I think that's a bad design, when something bad happens, the user should learn about it ASAP, not weeks later when they find out that their database is full of bogus data (or whatever).


sb = null;
sb = new StringBuilder();

That first line is completely unnecessary. If you're going to set the field to a new value anyway, setting it to null first won't change anything.


I don't understand the design of that class. What is the purpose of StartString and EndString? Practically, both do exactly the same thing, because of the null check in AddString().

You could provide just a single Reset() method that sets sb to a new StringBuilder and that would be enough. You could also remove the null check from AddString(), because sb would never be null.

Or even better, make MyNETClass single use. Instead of resetting, you would just create a new instance.

\$\endgroup\$
  • \$\begingroup\$ Hi Svick, Thanks for your comments, the code is mainly just demonstration for the purposes of the question infinite while loops and lack of proper error handling will not be part of production. \$\endgroup\$ – Keith Oct 2 '14 at 8:30
  • \$\begingroup\$ My main question is in relation to the Memory leaks and how I can prevent them. How does C++ now when to clean up the variables & when does the C# app clean up? \$\endgroup\$ – Keith Oct 2 '14 at 8:32
  • \$\begingroup\$ @Keith please avoid dumbed-down example code on this site in the future. Code Review isn't Stack Overflow, reviewers often spend a significant amount of time on CR questions, getting your actual real code peer reviewed is the purpose of this site. \$\endgroup\$ – Mathieu Guindon Oct 3 '14 at 1:36
  • \$\begingroup\$ Apologies @Mat'sMug The last time posted Full source files to ask a specific question on Stack Overflow it got moved to codereview so I assumed this would be the same \$\endgroup\$ – Keith Oct 3 '14 at 8:48
1
\$\begingroup\$

This code has some potential problems

void AddValue(long i) {
  char numstr[21] = {0}; // enough to hold all numbers up to 64-bits
  sprintf(numstr, "%ld", i);

  string s = string("Some String Here ") + string(numstr);

it can be written like this instead

string s = string("Some String Here ") + std::to_string(i);

if your to_string doesn't function you can do it this way instead

  static_assert(sizeof(i) == 8, "i is not 64 bit");
  constexpr int bufl = 1+sizeof(i)*3; // sign + 3 digits per 8 bit
  char numstr[bufl] = {0}; // enough to hold all numbers up to 64-bits
  snprintf(numstr, bufl, "%ld", i); // better safe than sorry

Garbage collection doesn't mean return to the OS as one of the other answers already stated. Here is how to force garbage collection, call it a couple of times to ensure it cleans everything up, then run your code again and see if you get to reuse the GC space.

\$\endgroup\$
  • \$\begingroup\$ @Keith, added a link to force GC. \$\endgroup\$ – Surt Oct 3 '14 at 10:01

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.