List iterator not incrementable for published event

[hal1001]

I have an event bus that I'm using to publish events to subscribers. However, I'm finding that after I unsubscribe a subscriber successfully the publish method is already mid-delivery for the same event that just got delivered. It was my understanding that event polling was all on the same thread as the window, but I'm probably just not understanding it correctly. Here is some of my code:

EventBus.cpp

#include "engine/events/EventBus.hpp"

void EventBus::unsubscribe(EventSubscriber* eventSubscriber)
{
        std::list<EventSubscriber*>::iterator iterator;
        for (iterator = subscribers.begin(); iterator != subscribers.end(); ++iterator)
        {
                if (*iterator == eventSubscriber)
                {
                        subscribers.erase(iterator);
                        break;
                }
        }
}

void EventBus::subscribe(EventSubscriber* eventSubscriber)
{
        subscribers.push_back(eventSubscriber);
}

void EventBus::publish(Game* game, sf::Event e)
{
        std::list<EventSubscriber*>::iterator iterator;
        for (iterator = subscribers.begin(); iterator != subscribers.end(); iterator++) {
                (*iterator)->deliver(game, e);
        }
}

std::list<EventSubscriber*> EventBus::subscribers = {};

EventBus.cpp has all static members.

Button.cpp (partial)

void Button::deliver(Game* game, sf::Event e)
{
        switch (e.type)
        {
        case sf::Event::MouseButtonReleased:
       
                if (e.mouseButton.button == sf::Mouse::Left && m_text.getGlobalBounds().contains(sf::Mouse::getPosition(m_window).x, sf::Mouse::getPosition(m_window).y))
                {
                        EventBus::unsubscribe(this);
                        game->newGame();
                }
               
                break;
       
        case sf::Event::MouseMoved:
               
                if (m_text.getGlobalBounds().contains(sf::Mouse::getPosition(m_window).x, sf::Mouse::getPosition(m_window).y))
                {
                        if (handCursor)
                        {
                                SetCursor(handCursor);
                        }
                }
                else
                {
                        if (arrowCursor)
                        {
                                SetCursor(arrowCursor);
                        }
                }
               
                break;
        }
}

Before the new game is created I unsubscribe the "New Game" button, and then create the game. It does remove the button from the list of subscribers, but if I step out after that point it immediately jumps to this line in the publish method of EventBus.cpp:

(*iterator)->deliver(game, e);

The event it is delivering is the MouseButtonReleased, which was already delivered to the button, or else I wouldn't have been able to call unsubscribe. This throws the runtime exception "List iterator not incrementable". My event handling is pretty standard.

Game.cpp (partial)

void Game::handleEvents()
{
        sf::Event event;
        while (m_window.pollEvent(event))
        {
                EventBus::publish(this, event);

                switch (event.type)
                {
                case sf::Event::Closed:

                        exit();
                        break;

                case sf::Event::Resized:

                        // TODO: Resize view.
                        break;

                case sf::Event::LostFocus:

                        pause();
                        break;

                case sf::Event::GainedFocus:

                        pause();
                        break;

                case sf::Event::KeyPressed:

                        if (event.key.code == sf::Keyboard::Escape)
                        {
                                exit();
                        }
                        else if (event.key.code == sf::Keyboard::P)
                        {
                                pause();
                        }

                        break;

                default:
                        break;
                }
        }
}

That method is called in my main loop.

I'm drawing a blank on why this control flow would be incorrect, or what is wrong with my C++ code. Any help is appreciated. Thanks!

Brian

[hal1001]

I made the following change:

void EventBus::publish(Game* game, sf::Event e)
{
    std::list<EventSubscriber*>::iterator iterator;
    for (iterator = subscribers.begin(); iterator != subscribers.end();) {
        (*iterator++)->deliver(game, e);
    }
}

I'm still not understanding why this is necessary though. The subscriber should no longer be in the list by the time EventBus::publish is called again. However, that change did fix it so I'm okay to move along.

[Jesper Juhl]

Suggestion:

for (auto& subscriber : subscribers)
  subscriber.deliver(game, e);

[Xornand]

You were removing the iterator from inside of the iteration and then trying to increment it. At that point the iterator was no longer valid, thus you were getting the error.

Out of curiosity, is there any compelling reason on why you're using std::list as opposed to std::vector?

[hal1001]

Suggestion:

for (auto& subscriber : subscribers)
  subscriber.deliver(game, e);

That was similar to my original implementation. I changed it to use an iterator in an attempt to fix the error I was seeing originally.

[hal1001]

You were removing the iterator from inside of the iteration and then trying to increment it. At that point the iterator was no longer valid, thus you were getting the error.

Out of curiosity, is there any compelling reason on why you're using std::list as opposed to std::vector?

That's just it though, there should have been no iteration at all. It should have seen that subscribers was empty, and not attempted to iterate. Admittedly I'm coming from the Java world so I may be missing a nuance of how the iterator here works -- that's the same excuse as to why I use a list -- it's a pretty standard fallback collection, but if there are better reasons to using a vector then I am happy to switch.

[Strelok]

void EventBus::unsubscribe(EventSubscriber* eventSubscriber)
{
    std::list<EventSubscriber*>::iterator iterator;
    for (iterator = subscribers.begin(); iterator != subscribers.end(); ++iterator)
    {
        if (*iterator == eventSubscriber)
        {
            subscribers.erase(iterator);
            break;
        }
    }

This should work

void EventBus::unsubscribe(EventSubscriber& eventSubscriber)
{
        subscribers.remove_if([&](EventSubscriber* value){return value == &eventSubscriber; });
}

[Xornand]

You were removing the iterator from inside of the iteration and then trying to increment it. At that point the iterator was no longer valid, thus you were getting the error.

Out of curiosity, is there any compelling reason on why you're using std::list as opposed to std::vector?

That's just it though, there should have been no iteration at all. It should have seen that subscribers was empty, and not attempted to iterate. Admittedly I'm coming from the Java world so I may be missing a nuance of how the iterator here works -- that's the same excuse as to why I use a list -- it's a pretty standard fallback collection, but if there are better reasons to using a vector then I am happy to switch.

You're using std::list, which is implemented as doubly-linked list under the cover.

Generally, std::vector is the "default" container in C++, which is Java's equivalent of ArrayList. Unless you're doing lots of insertions/deletions in the beginning or the middle, there's not much reason to use std::list. Also, std::vector is more L2 cache friendly while iterating.

[hal1001]

Awesome, thanks for the helpful tip!