boost interprocess and valgrind

Question

Here is a piece of code I am using to allocate map on shared memory, I am using boost::interprocess and managed shared memory segment, now the problem is I have encountered a memory leak. Given below is the top output.

top output:

PID USER      PR  NI  VIRT  RES  SHR S %CPU %MEM    TIME+  COMMAND
1.  27594 tpmon     20   0 46132 2140 1664 S  0.0  0.0   0:00.00 test_stub
2.  27594 tpmon     20   0 46132 2176 1664 S  0.0  0.0   0:00.01 test_stub
3.  27594 tpmon     20   0 46264 2248 1664 S  0.0  0.0   0:00.01 test_stub
4.  27594 tpmon     20   0 46264 2280 1664 S  0.0  0.0   0:00.01 test_stub

from the top output it is evident that the resident memory is continually increasing, in the shared memory map I have only the entries listed below,as triplets:

Deb0 0 150520 Deb1 1 150520 Deb10 10 150520 Deb11 11 150520 Deb12 12 150520 Deb13 13 150520 Deb14 14 150520 Deb15 15 150520 Deb16 16 150520 Deb17 17 150520 Deb18 18 150520 Deb19 19 150520 Deb2 2 150520 Deb20 20 150520 Deb21 21 150520 Deb22 22 150520 Deb23 23 150520 Deb24 24 150520 Deb25 25 150520 Deb26 26 150520 Deb27 27 150520 Deb28 28 150520 Deb29 29 150520 Deb3 3 150520 Deb4 4 150520 Deb5 5 150520 Deb6 6 150520 Deb7 7 150520 Deb8 8 150520 Deb9 9 150520

And these don't get added any further they just get updated.

The next step that I took was to run the valgrind as follows:

sudo -u tpmon valgrind --tool=memcheck --leak-check=yes ./bin/test_stub

And below is the output:

==21404== Memcheck, a memory error detector
==21404== Copyright (C) 2002-2012, and GNU GPL'd, by Julian Seward et al.
==21404== Using Valgrind-3.8.1 and LibVEX; rerun with -h for copyright info
==21404== Command: ./bin/test_stub
==21404==
Finished initializing TickerInfo Manager
^C==21404==
==21404== HEAP SUMMARY:
==21404==     in use at exit: 60,627 bytes in 1,264 blocks
==21404==   total heap usage: **5,059 allocs, 3,795 frees**, 812,123 bytes allocated
==21404==
==21404== 29 bytes in 1 blocks are possibly lost in loss record 2 of 7
==21404==    at 0x4A075BC: operator new(unsigned long) (vg_replace_malloc.c:298)
==21404==    by 0x3A7149C3C8: std::string::_Rep::_S_create(unsigned long, unsigned long, std::allocator<char> const&) (in /usr/lib64/libstdc++.so.6.0.13)
==21404==    by 0x3A7149CDE4: ??? (in /usr/lib64/libstdc++.so.6.0.13)
==21404==    by 0x3A7149CF32: std::basic_string<char, std::char_traits<char>, > std::allocator<char> >::basic_string(char const*, std::allocator<char> const&) (in /usr/lib64/libstdc++.so.6.0.13)
==21404==    by 0x40986F: main (test_stub.cxx:12)

From the valgrind output it is obvious that the number for allocs are greater than free, but does that have any real meaning, in case of shared memory we want the allocated memory to be present event after the process has exited.

test_stub.cxx

#include <stdio.h>
#include <iostream>
#include<string>
#include <sstream>

using namespace std;
int main() {

  TickerInfoManager * ticker_info_manager_inst = TickerInfoManager::get_instance("test");

  char_allocator ca(ticker_info_manager_inst->get_managed_memory_segment().get_allocator<char>());
  while(1) {
    for( int i=0; i < 30; i++ ) {
      basic_time now;
      stringstream convert;
      convert << i;
      int curr_time = now.fullTime();
      ticker_info_manager_inst->put_records( *(new tickerUpdateInfo(const_cast<char*>(("Deb"+convert.str()).c_str()), i, curr_time, ca ) ));
    }
    sleep(1);
  }
  //ticker_info_manager_inst->print_contents();
  return 0;
}

TickerInfoManager.cxx

#include <TickerInfoManager.h>
#include <TickerInfoMangerImplementation.h>

TickerInfoManager::TickerInfoManager( const sharedMemoryNameT & name) : pInfoMangerImpl( new tickerInfoMangerImplementation( name )) {

}

TickerInfoManager* TickerInfoManager::get_instance( const sharedMemoryNameT & name ) {
  return (new TickerInfoManager( name ) );
}

bool TickerInfoManager::put_records( const tickerUpdateInfoT & record ) {
    return pInfoMangerImpl->put_records( record );
}

void TickerInfoManager::print_contents() {
  return pInfoMangerImpl->print_contents();
}

bip::managed_shared_memory& TickerInfoManager::get_managed_memory_segment() {
  return pInfoMangerImpl->get_managed_memory_segment();
}

TickerInfoMangerImplementation.cxx

#include <TickerInfoMangerImplementation.h>
#include <boost/interprocess/managed_shared_memory.hpp>
#include <iostream>
#include "basic_time.h"

using namespace boost::interprocess;

tickerInfoMangerImplementation::tickerInfoMangerImplementation( const sharedMemoryNameT & name ): m_name(name),
  m_managed_memory_segment( open_or_create, "test", 1000000 ),
  p_ticker_info_map( m_managed_memory_segment.find_or_construct<KeyTickerCountMap>("TickerInfoMap")(std::less<SharedString>(), m_managed_memory_segment.get_segment_manager() ) ) {
    std::cout<<"Finished initializing TickerInfo Manager" << std::endl;
  }

bool tickerInfoMangerImplementation::put_records( const tickerUpdateInfoT & record ) {

  //If the key has not been inserted, insert the key and update the map

  KeyTickerCountMap::iterator iterator_to_map = p_ticker_info_map->find( record.m_id );

  if( iterator_to_map == p_ticker_info_map->end() ) {
    p_ticker_info_map->emplace( record.m_id, std::make_pair( record.m_total_ticker_count, record.m_active_ticker_count ) );
  }
  else {
    p_ticker_info_map->at(record.m_id)  = std::make_pair( record.m_total_ticker_count, record.m_active_ticker_count) ;
  }
  //record.m_ca.deallocate( const_cast<char*> ((record.m_id).c_str()), record.m_id.length() );
  return true;
}

int tickerInfoMangerImplementation::calculate_historical_time_using_threshold( const thresholdT seconds ) {

  basic_time::Secs_t secs( seconds );
  basic_time tick_time;
  tick_time -= secs;
  return ( tick_time.fullTime() );
}


void tickerInfoMangerImplementation::print_contents() {

  KeyTickerCountMap::iterator map_iter = (*p_ticker_info_map).begin();
  KeyTickerCountMap::iterator map_end  = (*p_ticker_info_map).end();

  for ( ; map_iter != map_end; ++map_iter ) {
    std::cout<< map_iter->first << " " << map_iter->second.first << " " << map_iter->second.second << std::endl;
  }
}

TickerInfo.h

    #ifndef __TICKER_INFO__
    #define __TICKER_INFO__

    #include <boost/interprocess/managed_shared_memory.hpp>
    #include <boost/interprocess/allocators/allocator.hpp>
    #include <boost/interprocess/containers/string.hpp>
    #include <iostream>

    typedef boost::interprocess::managed_shared_memory::allocator<char>::type               char_allocator;
    typedef boost::interprocess::basic_string<char, std::char_traits<char>, char_allocator> shm_string;

    //Data to insert in shared memory
    typedef struct tickerUpdateInfo {

      shm_string     m_id;
      int            m_total_ticker_count;
      int            m_active_ticker_count;
      char_allocator m_ca;

        tickerUpdateInfo( char * id,
            int total_ticker_count,
            int active_ticker_count,
            const char_allocator &a )
        : m_id( id, a),
        m_total_ticker_count(total_ticker_count),
        m_active_ticker_count(active_ticker_count),
        m_ca(a) {
        }

        ~tickerUpdateInfo() {
          std::cout<< "Calling destructor" <<std::endl;
        }

      tickerUpdateInfo& operator=(const tickerUpdateInfo& other) {
        if (this != &other) {
          m_total_ticker_count  = other.m_total_ticker_count;
          m_active_ticker_count = other.m_active_ticker_count;
        }
        return *this;
      }
    } tickerUpdateInfoT;

    #endif

**TickerInfoManager.h**

#ifndef __TICKER_INFO_MANAGER__
#define __TICKER_INFO_MANAGER__
#include <TickerInfoManagerConstants.h>
#include <TickerInfoMangerImplementation.h>

//class tickerInfoMangerImplementation;

class TickerInfoManager {

  public:
    static TickerInfoManager* get_instance( const sharedMemoryNameT & name );
    bool put_records( const tickerUpdateInfoT & record );
    TickerInfoManager( const sharedMemoryNameT & name);
    void print_contents();
    boost::interprocess::managed_shared_memory& get_managed_memory_segment();

  private:
    std::auto_ptr<tickerInfoMangerImplementation>  pInfoMangerImpl;
};

#endif

TickerInfoMangerImplementation.h

#ifndef __TICKER_INFO_MANAGER_IMPL__
#define __TICKER_INFO_MANAGER_IMPL__

#include <boost/interprocess/allocators/allocator.hpp>
#include <boost/interprocess/managed_shared_memory.hpp>
#include <boost/interprocess/containers/string.hpp>
#include <boost/interprocess/containers/map.hpp>
//#include <TickerInfoManagerConstants.h>
#include <TickerInfo.h>
#include <vector>
#include <fire/HashMap.h>
#include <string>

 typedef std::string sharedMemoryNameT;
 typedef int         thresholdT;


namespace bip = boost::interprocess;


//the strings also need to be assigned from the shared memory
typedef bip::allocator<void, bip::managed_shared_memory::segment_manager> VoidAllocator;
typedef bip::allocator<char, bip::managed_shared_memory::segment_manager> CharAllocator;
typedef bip::basic_string<char, std::char_traits<char>, CharAllocator> SharedString;

//Note that map<Key, MappedType>'s value_type is std::pair<const Key, MappedType>,
//so the allocator must allocate that pair.
typedef bip::allocator<std::pair<const SharedString, std::pair<int,int> >, bip::managed_shared_memory::segment_manager> MapValueTypeAllocator;
typedef bip::map<SharedString, std::pair<int,int>, std::less<SharedString>, MapValueTypeAllocator>  KeyTickerCountMap;

//allocator for the string
typedef bip::allocator<SharedString, bip::managed_shared_memory::segment_manager> StringAllocator;

class tickerInfoMangerImplementation {

  public:
    tickerInfoMangerImplementation( const sharedMemoryNameT & name );

    bool put_records( const tickerUpdateInfoT & record );

    void print_contents();

    bip::managed_shared_memory& get_managed_memory_segment() {
      return m_managed_memory_segment;
    }


  private:
    const sharedMemoryNameT    m_name;
    bip::managed_shared_memory m_managed_memory_segment;
    bip::offset_ptr<KeyTickerCountMap> p_ticker_info_map;

    int calculate_historical_time_using_threshold( const thresholdT seconds );
};
#endif

So basically the code flows like this:

from test_stub.cxx we call put_records in TickerInfoManager ------> call put_records(in tickerInfoManagerImplementation)---> put_records in tickerInfoManagerImplementation inserts data to the map which resides in the shared memory.

I have added the complete code, if anybody wants to reproduce the situation.

My question is how do I go about debugging this problem, am I not understanding the valgrind output properly?

Thanks, Deb!

Answer 1

Warning: ranting ahead. I do get constructive and provide a constructive fixed version at the end.

I know this is not going to sound nice, but perhaps you should stop doing this in C++.

At the very least stop trying to emulate Java. Stop writing thread-unsafe code. Stop writing memory leaks.

How? Start simple.

You're aiming way too high here.

I've assisted you about 4 times now

• Keeping fixed size symbols in shared memory
• Program crashes when trying to access the inner map
• Using boost multi index like relational DB

In both these last answers I basically rewrote the whole thing and suggested you rethink the design.

Yet you manage to come back with /further/ complicated versions of already [...] code. Starting with the most important:

• You are using the memory-leak operator (*new)¹ in main():

  *(new tickerUpdateInfo(const_cast<char *>(("Deb" + convert.str()).c_str()), i, curr_time, ca)));
  

  Congratulations, because you also abused const_cast in the same line. Just take std::strings already, and don't be evil.

• None of your shared memory accesses are properly protected by locks. This means that all behaviour of your processes is undefined.

Beyond that, there's a wide range of issues with the code:

• tickerUpdateInfo::operator= violates value semantics by not assigning id (and allocator). (This operator is not used)

• In fact, I don't see the purpose of the whole tickerUpdateInfo class, except to make sure that the id string is allocated and deallocated more than necessary (oh, and of course to be leaked, see the first bullet).

• typedef struct X {...} X_t; is a C-ism. There is no need to tag structs in C++

• tickerUpdateInfo doesn't need to hold the allocator (it's a property of the string).

• Your TickerUpdateManager has get_instance. So, is it a singleton? Oh no, it can't be because it returns a new instance every time it's called. Oh, and the manager is just leaked too.

• The manager does nothing. All it does is delegate to an implementation. Pimpl idiom is nice, but only serves any purpose if it actually isolates the interface from the implementation. Your Manager, however, directly exposes the implementation types in the interface, and even directly exposes non-const references into implementation (get_segment_manager). That's an interface design no-no and Law-Of-Demeter violation.

  This Manager/Implementation distinction is nothing but noise and adds complexity

• Construction of TicketUpdateManagerImplementation doesn't guard construction of the shared memory or the TickerInfoMap with locks.

• auto_ptr is deprecated. Use std::unique_ptr (or boost::scoped_ptr if you must.). Of course, this is assuming that you even needed the pimpl there.

• m_name is never used (not even used to open the shared memory segment), and sharedMemoryNameT is snake-oil.

• std::less<SharedString> is the default comparator already

• put_records is an extremely roundabout way of doing:

  (*p_ticker_info_map)[record.m_id] = { record.m_total_ticker_count, record.m_active_ticker_count };
  

  Prefer concise code over repetition with flow control logic. Also, why is there a true return value?

• calculate_historical_time_using_threshold is an interesting name. The function does not belong in this class, and doesn't not calculate a historical time. Oh, and it doesn't use any threshold. (AFAICT it returns the full-time seconds ago.)

• you have competing typedefs for shm_string vs. SharedString (with their respective allocators).

• There's no need to put all the implementation details (typedefs) into the global namespace, and in the header file.

Cleanup

Here's a proposed cleanup of the manager, with pimpl done right. See how we just use standard library types in the header:

#pragma once

#include <string>
#include <memory>

class TickerInfoManager {

  public:
    TickerInfoManager(const char* name);
    ~TickerInfoManager();

    void put(std::string const& id, int total, int active);
    void print_contents() const;

  private:
    struct Impl;
    std::unique_ptr<Impl> pimpl_;
};

The main program is similarly much simpler now:

#include <iostream>
#include <string>
#include "TickerInfoManager.hxx"
#include "basic_time.h"
#include <thread>

int main() {
    TickerInfoManager tickerinfo("test");

    while (1) {
        for (int i = 0; i < 30; i++) {
            tickerinfo.put("Deb" + std::to_string(i), i, basic_time().fullTime());
        }

        std::this_thread::sleep_for(std::chrono::milliseconds(100)); // sleep(1)
        std::cout << "." << std::flush;
    }
}

The implementation was made thread-safe with IPC synchronization and concentrates all the Boost Interprocess related stuff:

#include "TickerInfoManager.hxx"
#include <iostream>

#include <boost/interprocess/sync/interprocess_mutex.hpp>
#include <boost/interprocess/allocators/allocator.hpp>
#include <boost/interprocess/managed_shared_memory.hpp>
#include <boost/interprocess/containers/string.hpp>
#include <boost/interprocess/containers/map.hpp>
#include <boost/thread/lock_guard.hpp>

namespace /*anon*/ {
    namespace bip = boost::interprocess;

    typedef bip::allocator<char, bip::managed_shared_memory::segment_manager> CharAllocator;
    typedef bip::basic_string<char, std::char_traits<char>, CharAllocator> SharedString;
    typedef bip::allocator<std::pair<SharedString const, std::pair<int, int> >, bip::managed_shared_memory::segment_manager> MapValueTypeAllocator;
    typedef bip::map<SharedString, std::pair<int, int>, std::less<SharedString>, MapValueTypeAllocator> KeyTickerCountMap;
    typedef boost::lock_guard<bip::interprocess_mutex> lock_guard;

    struct LockableMap {
        LockableMap(MapValueTypeAllocator alloc) : map(alloc) {}

        KeyTickerCountMap       map;
        bip::interprocess_mutex mutex;
    };
}

struct TickerInfoManager::Impl {
    Impl(const char* name)
        : m_segment(bip::open_or_create, name, 1000000),
          m_alloc(m_segment.get_segment_manager()),
          p_data(m_segment.find_or_construct<LockableMap>("TickerInfoMap")(m_segment.get_segment_manager())) 
    {}

    bip::managed_shared_memory   m_segment; // order is relevant
    CharAllocator                m_alloc;
    bip::offset_ptr<LockableMap> p_data;

    KeyTickerCountMap&       map()   { return p_data->map;   }
    bip::interprocess_mutex& mutex() { return p_data->mutex; }
};

TickerInfoManager::TickerInfoManager(const char* name) : pimpl_(new Impl(name)) { }

TickerInfoManager::~TickerInfoManager() { }

void TickerInfoManager::put(std::string const& id, int total, int active) {
    SharedString shid(id.begin(), id.end(), pimpl_->m_alloc);

    lock_guard lock(pimpl_->mutex());
    pimpl_->map()[shid] = { total, active };
}

void TickerInfoManager::print_contents() const {
    lock_guard lock(pimpl_->mutex());
    for (auto const& e : pimpl_->map())
        std::cout << e.first << " " << e.second.first << " " << e.second.second << std::endl;
}

All in all, it's less than half the amount of code, while it does

• properly hide implementation details
• properly synchronizes access to the shared memory
• properly manage memory

See the code Compiling On Coliru

---

¹ C++ program crashes after creating 2000 objects