-1

I'm having a sporadic problem right now where map::erase() says it worked, but the map sometimes still has the element in it.

auto iterator = device_map.begin();  // std::map<std::string,Device*>
size_t nDeleted = 0;
while (iterator != map.end()) {
    Device* device = device_map[iterator->first];
    if (device->done()) {
        device->close();
        cout << "Erasing " << iterator->first << endl;
        nDeleted = device_map.erase(iterator->first);
        delete device;
    }
    ++iterator;
}

This is called in a std::thread every 500ms. I've tested it by printing the contents of the map before and after the loop, like this:

cout << "device_map = ";
for (std::map<string, Device*>::iterator it = device_map.begin(); it != device_map.end(); ++it) {
    cout << it->first << " = " << it->second << "; ";
}
cout << endl;

Sometimes, even if nDeleted == 1, the contents of device_map is the same before and after unmapping the element (except that *device == nullptr because of the delete).

I've also put a breakpoint and device_map.size() is indeed unchanged after calling device_map.erase() does not work.

I have tried this other version of map::erase with the same results:

auto iterator = device_map.begin();  // std::map<std::string,Device*>
size_t nDeleted = 0;
while (iterator != map.end()) {
    Device* device = device_map[iterator->first];
    if (device->done()) {
        device->close();
        cout << "Erasing " << iterator->first << endl;
        iterator = device_map.erase(iterator);
        delete device;
    } else {
        ++iterator;
    }
}

Is there something that I'm doing wrong?

Thank you,

Fred

fchasse
  • 11
  • 2
  • 5
    You're deleting your elements whilst iterating over your map, once you delete an entry you invalidate the iterator. Dupe: https://stackoverflow.com/questions/8234779/how-to-remove-from-a-map-while-iterating-it. Basically your map has to re-sort once you modify the contents – EdChum Sep 06 '19 at 14:33
  • 7
    `Device* device = device_map[iterator->first];` This is silly. You should use `... = iterator->second`. – HolyBlackCat Sep 06 '19 at 14:34
  • 2
    @HolyBlackCat `device_map.erase(iterator->first);` also "great" approach, especially that `device_map.erase(iterator)` would compile. – Slava Sep 06 '19 at 14:37
  • @EdChum, the code updates the iterator correctly: iterator = device_map.erase(iterator); and tests equality with end() called every time. I believe the looping/editing is fine – Jeffrey Sep 06 '19 at 14:38
  • 1
    @fchasse, did you try putting the whole loop into a mutex ? Are you sure you have a single thread playing with this data at once? – Jeffrey Sep 06 '19 at 14:40
  • @Jeffrey I was looking at the first code snippet, the second snippet does seem fine though. Maybe this is a synchronisation issue – EdChum Sep 06 '19 at 14:40
  • You wouldn’t happen to be using the map in a different thread as well? – molbdnilo Sep 06 '19 at 14:40
  • 3
    Please post a [mcve]. We don't even know if the map you're erasing from is the actual map you want to erase from. There are many questions here where a *copy* of the map is being worked with instead of the original, thus any changes to the original are not being done. – PaulMcKenzie Sep 06 '19 at 14:46
  • Not a duplicate, as discussed in the comments above. Second snippet is fine as far as deletion go. – Jeffrey Sep 06 '19 at 14:52
  • @Jeffrey I will try, but this specific map is used by only one thread – fchasse Sep 06 '19 at 17:11
  • Then something doesn't add up. Is it possible that device::done(), device::close() or device::~device() calls indirectly back into this code, or into anything that changes the map? – Jeffrey Sep 06 '19 at 17:15
  • @Jeffrey Actually, the map itself might be accessed by another thread _while_ map::erase is being called, would that explain the issue? There is a function in another thread that uses map::find and map::end, and I think it's possible that it happens during the erase – fchasse Sep 06 '19 at 17:15
  • It could explain the issue. std::map is not thread-safe on non-const operations – Jeffrey Sep 06 '19 at 17:17
  • I will add a mutex and post back the results – fchasse Sep 06 '19 at 17:20
  • It seems that adding a mutex solved the issue, thank you all! – fchasse Sep 06 '19 at 19:14

1 Answers1

0

As @Jeffrey suggested, the issue was with another thread accessing the std::map object while map::erase was being called. Adding a mutex solved the problem.

fchasse
  • 11
  • 2