std::list and garbage Collection algorithm

Question

I have a server that puts 2 players together on request and starts a game Game in a new thread.

struct GInfo {Game* game; std::thread* g_thread};

while  (true) {
    players_pair = matchPlayers();
    Game* game = new Game(players_pair);
    std::thread* game_T = new std::thread(&Game::start, game);
    GInfo ginfo = {game, game_T}
    _actives.push_back(ginfo);    // std::list
}

I am writing a "Garbage Collector", that runs in another thread, to clean the memory from terminated games.

void garbageCollector() {
    while (true) {
        for (std::list<Ginfo>::iterator it = _actives.begin(); it != _actives.end(); ++it) {
            if (! it->game->isActive()) {
                delete it->game; it->game = nullptr;
                it->g_thread->join();
                delete it->g_thread; it->g_thread = nullptr;
                _actives.erase(it);
            }
        }
        sleep(2);
    }
}

This generates a segfault, I suspect it is because of the _active.erase(it) being in the iteration loop. For troubleshooting, I made _actives an std::vector (instead of std::list) and applied the same algorithm but using indexes instead of iterators, it works fine.

Is there a way around this?

Is the algorithm, data structure used fine? Any better way to do the garbage collection?

Help is appreciated!

`_actives.erase(it)` _invalidates_ `it` and returns an iterator pointing to the next element. Also note that you should **not** do `++it` in that case, as you will advance two elements instead of one. — Botje, Mar 08 '19 at 12:21
Possible duplicate https://stackoverflow.com/questions/596162/can-you-remove-elements-from-a-stdlist-while-iterating-through-it — Quimby, Mar 08 '19 at 12:22

score 1 · Answer 1 · answered Mar 08 '19 at 12:21

If you have a look at the documentation for the erase method it returns an iterator to the element after the one that was removed.

The way to use that is to assign the returned value to your iterator like so.

for (std::list<Ginfo>::iterator it = _actives.begin(); it != _actives.end();) {
    if (! it->game->isActive()) {
        delete it->game; it->game = nullptr;
        it->g_thread->join();
        delete it->g_thread; it->g_thread = nullptr;
        it = _actives.erase(it);
    }
    else {
        ++it;
    }
}

Since picking up the return value from erase advances the iterator to the next element, we have to make sure not to increment the iterator when that happens.

On an unrelated note, variable names starting with underscore is generally reserved for the internals of the compiler and should be avoided in your own code.

Quimby · Answer 2 · 2019-03-08T12:44:52.660

Any better way to do the garbage collection?

Yes, don't use new,delete or dynamic memory alltogether:

struct Players{};
struct Game{
    Game(Players&& players){}
};
struct GInfo {
    GInfo(Players&& players_pair):
        game(std::move(players_pair)),g_thread(&Game::start, game){}

    Game game; 
    std::thread g_thread;
};

std::list<GInfo> _actives;

void someLoop()
{
    while  (true) {
        GInfo& ginfo = _actives.emplace_back(matchPlayers());
    }
}

void garbageCollector() {
    while (true) {
        //Since C++20
        //_active.remove_if([](GInfo& i){ return !i.game.isActive();});
        //Until C++20
        auto IT =std::remove_if(_actives.begin(),_actives.end(),
                      [](GInfo& i){ return !i.game.isActive();});
        _active.erase(IT,_active.end());
        //
        sleep(2);
    }
}

There might be a few typos, but that's the idea.

std::list and garbage Collection algorithm

2 Answers2