call()() memory leak?

ID:2227180
 
Mar 21 2017, 4:07 pm
Resolved
call() had a memory leak when calling an external library with an arglist.
BYOND Version:511.1377
Operating System:Windows 7 Pro 64-bit
Web Browser:Chrome 56.0.2924.87
Applies to:DM Language
Status: Resolved (511.1380)
This issue has been resolved.
Descriptive Problem Summary:
#ifndef LIBNQUTILITY_DLL_WIN32
#define LIBNQUTILITY_DLL_WIN32 "./libnqutility.dll"
#endif

#ifndef LIBNQUTILITY_DLL_UNIX
#define LIBNQUTILITY_DLL_UNIX "./libnqutility.so"
#endif

/var/nq_utility/nq_utility = new

/nq_utility/proc/CallProc(function, ...)
    var/list/L                                      = args.Copy(2)

    //return call(world.system_type == MS_WINDOWS ? LIBNQUTILITY_DLL_WIN32 : LIBNQUTILITY_DLL_UNIX, function)(arglist(L))
    . = call(world.system_type == MS_WINDOWS ? LIBNQUTILITY_DLL_WIN32 : LIBNQUTILITY_DLL_UNIX, function)(arglist(L))
    del(L)

In the above case, call()() holds onto the list L. Only a hard-delete rids the list.

Numbered Steps to Reproduce Problem:
1. Install NullQuery.Common Operations in a project
2. Execute Random() a few times (found in nq_rand.dm)
3. Observe the memory for lists

Expected Results: I expected the list L to be garbage collected after the proc has ended/returned.

Actual Results: The list L remains in memory indefinitely.

Does the problem occur:
Every time? Or how often? Every time
In other games? Yes
In other user accounts? N/A
On other computers? N/A

When does the problem NOT occur? N/A

Workarounds: N/A
Mar 22 2017, 9:11 am
Created a test case to possibly get this looked into faster.

Run the project, use the "test" verb, observe the memory stat for lists.
Mar 22 2017, 9:28 am
Worth pointing out here is that the .dll in the library itself has a memory leak. It creates buffers but does not ever free them. (It ought to free them on a subsequent call.)
Mar 22 2017, 10:01 am
In response to Lummox JR
Is that the cause of the issue I'm referring to, or a separate issue entirely?

If the former, what would I do to fix it?
Mar 22 2017, 10:14 am
I think it's a separate issue. I'll look into this soon and see what I can find. The leaks in the library won't have anything to do with the list not being let go.

The .dll/.so files need to be recompiled from altered source in order for their leak to go away. The source on Github shows that the buffers are being allocated but never deleted on a subsequent call.
Mar 22 2017, 10:41 am
Ah, alright. Much appreciated.
Apr 4 2017, 9:44 am (Edited on Apr 4 2017, 9:52 am)
Should I expect a fix for anytime soon... say within the next month? Currently have to resort to..

// using library... that's currently BYOND-broken.
var t = random.Random(0.5, 2.5)

// disabled library.
var t = pick(0.5, 0.75, 1, 1.25, 1.5, 1.75, 2, 2.25, 2.5)




On second thought, I've found a viable workaround:

. = list()
for(var/i = 0; i <= 5000; i += TICK_LAG)
    . += i
Apr 17 2017, 9:38 am
Lummox JR resolved issue with message:
call() had a memory leak when calling an external library with an arglist.
Apr 17 2017, 9:45 am
Don't forget you have to recompile the .dll/.so files if you want to fix their own memory leak. Most of the functions that return non-constant data use this pattern:

char *buf = new char[length];
...  // put some text in buf
return buf

The correct way to handle the buffers is to setup a global buffer that you allocate only when you need to create it the first time or add more space. E.g.,

char *global_buf = NULL;
int global_buflen = 0;
char *AllocBuf(int len) {
    if(len >= global_buflen) {
        global_buf = (char*)realloc(global_buf, len);
        global_buflen = global_buf ? len : 0;
    }
    return global_buf;
}

...

myfunction(int n, char const *arg[]) {
    ...
    buf = AllocBuf(length);
    if(!buf) return "";
    ... // fill buf
    return buf;
}
Apr 17 2017, 10:05 am
Nice! Appreciate the fix.

The buffer situation is new territory but it seems simple enough. I'll give it a shot.
Apr 17 2017, 11:54 am
The basic deal with the buffer is, you just use one for all output (except for constant strings). Whenever you call the DLL, you can assume the previous buffer contents have already been used and are no longer valid, so they can be overwritten.

The original library goes wrong by allocating a new buffer every time, but never deallocating it. By just using a single static buffer, the only time you have to worry about deallocating is when you're growing the buffer (allocating a bigger space after; realloc() handles all this for you), or when the .dll unloads--which will deallocate the buffer automatically.
Apr 17 2017, 12:47 pm
What program are you using to open the .dll file?
Apr 17 2017, 12:57 pm
I'm not using any program to open it. All I did was recompile it from the source on Github, except I added a test function of my own. My observation about the buffer problem comes only from looking at the library's source, where it's clear that buffers are being handled incorrectly.
Apr 17 2017, 2:33 pm
Hmm.. I was just trying to see the code so I could make the changes. No program I've downloaded has done exactly that though.

But I'm gonna leave it for now -- it's not hurting anything (or is it?) and too much time has been invested already.
Apr 17 2017, 3:13 pm
The source for that .dll is up on Github. It's linked right on the hub entry.
Apr 17 2017, 4:00 pm
Ohh, I see what you mean now.

So would I be wrong in thinking your solution is the only solution? Those specific functions return the buffer, so wouldn't going the deallocation route break said functions?

Apr 17 2017, 4:26 pm
Basically you should never have to deallocate if you're just using the one buffer. realloc() will grow it for you if you need to make it bigger.

With .dll calls you need the buffer to remain intact when the function returns, but the next time any function is called the buffer is then safe to deallocate if you wish. Using just a single buffer however makes the most sense.
Apr 17 2017, 4:33 pm
In response to Lummox JR
Ah, gotcha. I appreciate your time.