Beanstalkd: testing: fix memory leaks

[PatriciaSVMonteiro]

In the ctbenchheapinsert (testheap.c, line 222) a memory allocation is made for the variable j and it is not released at the end of the function, resulting in a memory leak type error.

void ctbenchheapinsert(int n)
{
    job *j;
    int i;
    j = calloc(n, sizeof *j);
    assert(j);
    for (i = 0; i < n; i++) {
        j[i] = make_job(1, 0, 1, 0, 0);
        assert(j[i]);
        j[i]->r.pri = -j[i]->r.id;
    }
    Heap h = {0};
    h.less = job_pri_less;
    h.rec = job_setheappos;
    ctresettimer();
    for (i = 0; i < n; i++) {
        heapinsert(&h, j[i]);
    }
}

[JensRantil]

Hi @PatriciaSVMonteiro! Nice find! Would you be interested in submitting a pull request to fix this?

[chadchristenson]

I think this is ok. heapinsert doesn't allocate memory for the object. It only allocates space on the heap for the pointer.

[sjinks]

Actually @PatriciaSVMonteiro is right, there is a leak. This is what valgrind says:

ctbenchheapinsert       ==1215== 
==1215== HEAP SUMMARY:
==1215==     in use at exit: 32,208 bytes in 61 blocks
==1215==   total heap usage: 118 allocs, 57 frees, 1,870,928 bytes allocated
==1215== 
==1215== 8 bytes in 1 blocks are definitely lost in loss record 1 of 6
==1215==    at 0x4C31B25: calloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==1215==    by 0x119DAC: ctbenchheapinsert (testheap.c:208)
==1215==    by 0x10BCD0: runbenchn (ct.c:324)
==1215==    by 0x10BF8E: runbench (ct.c:419)
==1215==    by 0x10C253: runallbench (ct.c:471)
==1215==    by 0x10C620: main (ct.c:560)
==1215== 
==1215== 16 bytes in 1 blocks are definitely lost in loss record 2 of 6
==1215==    at 0x4C2FB0F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==1215==    by 0x10EADB: heapinsert (heap.c:84)
==1215==    by 0x119F22: ctbenchheapinsert (testheap.c:220)
==1215==    by 0x10BCD0: runbenchn (ct.c:324)
==1215==    by 0x10BF8E: runbench (ct.c:419)
==1215==    by 0x10C253: runallbench (ct.c:471)
==1215==    by 0x10C620: main (ct.c:560)
==1215== 
==1215== LEAK SUMMARY:
==1215==    definitely lost: 24 bytes in 2 blocks
==1215==    indirectly lost: 0 bytes in 0 blocks
==1215==      possibly lost: 0 bytes in 0 blocks
==1215==    still reachable: 32,184 bytes in 59 blocks
==1215==         suppressed: 0 bytes in 0 blocks
==1215== Reachable blocks (those to which a pointer was found) are not shown.
==1215== To see them, rerun with: --leak-check=full --show-leak-kinds=all

testheap.c:208 is j = calloc(n, sizeof *j);
testheap.c:220 is heapinsert(&h, j[i]);

[ysmolsky]

There are so many more memory leaks in other tests, so I do not even know where to get started on this. Maybe it does not make sense to make tests leaks free and just have some kind of automated stress testing run on one instance of beanstalkd under valgrind inspection?

[sergeyklay]

Let me prepare a built/test matrix for Travis CI

[ysmolsky]

FYI, my fix is working. I have tested it on Linux manually:
MAKEFLAGS= valgrind --leak-check=full ct/_ctcheck -b >out 2>&1

MAKEFLAGS= hack is required because _ctcheck will fail if there is none. I will submit patch to ct soon. Sighs...
And you should not run "valgrind make bench -b", that won't produce what we want.

It would make sense to add option into ct to run only tests/benches matching some patters. @kr

[ysmolsky]

Let me prepare a built/test matrix for Travis CI

I would be awesome to have valgrind ran on linux via Travis, the output from it should not break the build too. That way we could peek what tests are leaking memory and fix them too. I guess.

valgrind on master: https://gist.github.com/ysmolsky/9a466cdac997f0b060d94cc59276fb46

[kr]

Is there a way to tell valgrind to ignore leaks of memory that was allocated in test files? It seems like a waste of effort to free memory immediately before the process exits when the OS will free that same memory automatically anyway, and it makes the test source code a little noisier, and it makes the tests a tiny bit slower.

[sergeyklay]

As a far as I know when valgrind runs its default tool, memcheck, it automatically tries to read a file called $PREFIX/lib/valgrind/default.supp ($PREFIX will normally be /usr). However we can make it use additional suppression files of your choice by adding --suppressions=<filename> to command-line invocation. For more see: http://valgrind.org/docs/manual/manual-core.html#manual-core.suppress

[ysmolsky]

Collecting right suppressions into file is tricky.
valgrind reports 18 places with definite memory leaks in tests. It means that only 18 tests are memory leaking, might be not so bad after all.

[kr]

That makes sense. If suppressing errors is tricky then I agree it would be better overall to just eliminate the leaks.

[ysmolsky]

So I was thinking about this. There are two kind of tests in beanstalkd: for structures without spawning child server, and with child process.

1st kind of tests: after removal of leaks we cannot still prove that there is no leakage in the main program because the absence of leak in a test does not mean that we forgot about calling free for the structure in the main program somewhere. How can we prove/test that?

2nd kind of tests assumes that server will be killed and the global structure will be cleaned by the OS. beanstalkd does not try to free memory when exiting. To fix these tests we need to introduce the way for beanstalkd to cleanup after itself.

I really do not want to spend time fixing these leaks just for the sake of non leaking tests. What is the purpose of fixing leaks? To prove that it's possible to not leak memory in tests? Also there is just too much of code with reliance on global state. Modifying some parts of the code is very tricky and feels like walking through the minefield.

Can anyone suggest some flaw in my reasoning? What else can be done to make automate leaks detection in the server?

🤷‍♂

[kr]

Is there a way to trigger a memcheck/valgrind report at an arbitrary time, well before the process is ready to exit? I would expect any leaks detected that way to be "real" because there should still be a meaningful root set.

For example, in a test that spawns a child server, the test function might signal the child process to generate a report after it runs the test logic, but before it kills the child. At that point, the child will be in socknext waiting for I/O. If memcheck finds a leak in the child (server) process at that point, it will be meaningful. Then the test function can kill the child process (or simply return) as before.

I think it's not possible to prove there are no leaks, but that's the same as unit testing in general. It's not possible for tests to prove correct behavior, they can only demonstrate incorrect behavior. They're still useful.

[ysmolsky]

I am not aware of that feature of valgrind. But maybe there is no need to do that.
If we eliminate all the known places with leaks (and make server to cleanup after itself), then valgrind will happily report newly detected ones when they happen. I have learned about valgrind - it is very good at finding memory bugs.

Anyway, y-day I got a bit weary from the initial push on beanstalkd and I was not thinking straight. Right now I see that cleaning leaks is a good way forward to improve the quality of a program and maybe rewrite some parts that are not clean-memory friendly.

I plan to work more deliberately from now on. I am going to improve tests and memory leaks in next weeks. But my pace will be slow compared to previous weeks, unfortunately.