Correctly disposing objects upon server termination

Question

I am working on a large C++ project. It consists in a server that exposes a REST API, providing a simple and user-friendly interface for a very broad system comprising many other servers. The codebase is quite large and complex, and evolved through time without a proper design upfront. My task is to implement new features and refactor/fix the old code in order to make it more stable and reliable.

At the moment, the server creates a number of long-living objects that are never terminated nor disposed when the process terminates. This makes Valgrind almost unusable for leak detection, as it is impossible to distinguish between the thousands of (questionably) legitimate leaks from the "dangerous" ones.

My idea is to ensure that all objects are disposed before termination, but when I made this proposal, my colleagues and my boss opposed me pointing out that the OS is going to free that memory anyway (which is obvious to everybody) and disposing the objects will slow down the shutdown of the server (which, at the moment, is basically a call to std::exit). I replied that having a "clean" shutdown procedure does not necessarily imply that one must use it. We can always call std::quick_exit or just kill -9 the process if we feel impatient.

They replied "most Linux daemons and processes don't bother freeing up memory at shutdown". While I can see that, it is also true that our project does need accurate memory debugging, as I already found memory corruption, double frees and uninitialised variables.

What are your thoughts? Am I pursuing a pointless endeavour? If not, how can I convince my colleagues and my boss? If so, why, and what should I do instead?

Answer 1

Add a switch to the server process that can be used during valgrind measurements that will release all of the memory. You can use this switch for testing. The impact will be minimal during normal operations.

We had a long running process that would take several minutes to releases 1000’s of objects. It was much more efficient to just exit and let them die. Unfortunately, as you indicate, this made it difficult to detect true memory leaks with valgrind or any other tools.

This was a good compromise for our testing while not impacting normal performance.

Answer 2

The key here is this:

While I can see that, it is also true that our project does need accurate memory debugging, as I already found memory corruption, double frees and uninitialised variables.

This pretty much directly implies that your codebase is cobbled together from nothing more than hope and string. Competent C++ programmers do not have double frees.

You absolutely are pursuing a pointless endeavour- as in, you are addressing one tiny symptom of the actual problem, which is that your code is about as reliable as the Apollo 13 service module.

If you program your server correctly with RAII, these issues will not occur, and the problem in your question will be eliminated. Plus, your code might actually execute correctly from time to time. Thus, it's clearly the best option.

Answer 3

One good approach would be to narrow down the discussion with your colleagues by means of classification. Given a large code base, certainly there is not one single reason but rather multiple, (identifiable) reasons for long living objects.

Examples:

• Long living objects which are not referenced by anyone (real leaks). It is a programming logic error. Fix those with lower priority UNLESS they are responsible for your memory footprint to grow over time (and deteriorate your applications quality). If they make your memory footprint grow over time fix them with higher priority.

• Long living objects, which are still referenced but not used anymore (due to program logic), but which do not make your memory footprint grow. Code review and try to find the other bugs which lead to that. Add comments to the code base if it is an intentional (performance) optimization.

• Long living objects "by design". Singleton pattern for example. They are indeed hard to get rid of, especially if it is a multi- threaded application.

• Recycled objects. Long living objects need not always be bad. They can also be beneficial. Instead of having high frequency memory allocations/deallocations, adding currently unused objects to a container to draw from when such a memory block is needed again can help speed up an application and avoid heap fragmentation. They should be easy to free at shutdown time, maybe in a special "instrumentation/checked" build.

• "shared objects" - objects which are used (referenced) by multiple other objects and no one knows exactly when it is save to free them. Consider turning them into reference counted objects.

Once you have classified the real reasons for those unfreed objects, it is much easier to enter a case by case discussion and find a resolution.

Answer 4

IMHO, the lifetimes of these objects should never just be made and left to die when the system shuts down. This reeks of global variables, which we all know are bad bad bad bad bad. Especially in the age of smart pointers, there is no reason to do this other than laziness. But more importantly, it adds a level of technical debt to your system that someone may have to deal with some day.

The idea of "technical debt" is that when you take a shortcut like this, when someone wants to change the code in the future (lets say, I want to be able to make the client go into "offline mode" or "sleep mode", or I want to be able to switch servers without restarting the process) they will have to put the effort in to do what you are skipping doing. But they will be maintaining your code, so they won't know nearly as much about it as you do, so it will take them much longer (I'm not talking 20% longer, i'm talking 20x longer!). Even if it is you, then you're going to have not worked on this particular code for weeks or months, and it'll take much longer to dust off the cobwebs in order to implement it correctly. The increase in time that needs to be spent to deal with bad design decisions is the interest on the technical debt.

In this case, it appears you have very tight coupling between the server object and the "long lived" objects...there are times when a record could (and should) outlive the connection to the server. Maintaining every single change to an object in a server can be prohibitively expensive, so its usually better that the spawned objects really be chached handles to the server object, with save and update calls that actually change the server. This is commonly called the active record pattern. See:

http://en.wikipedia.org/wiki/Active_record_pattern

In C++, i'd have each active record have a weak_ptr to the server, which could throw things intelligently if the server connection goes dark. These classes can be either lazily or batch populated, depending on your needs, but the lifetime of those objects should only be where they are used.

See also:

Is it a waste of time to free resources before I exit a process?

The other

Answer 5

If you can easily identify where the objects that should survive indefinitely are allocated, once possibility would be to allocate them using an alternative allocation mechanism so that either they don't show up in a valgrind leak report or just appear to be a single allocation.

In case you're not familiar with the idea, here's an article on how to impotent custom memory allocation in c++, although note that your solution could well be simpler than the examples in that article as you don't need to handle deletion at all!