Use vector in multithreading Exception thrown: read access violation

Question

Using a vector in multithreading, an read access violation is thrown.

I guess it may be a memory access related problem, but I don’t know how to solve it.

Below is my code:

doSomeThing.h

#ifndef _DOSOMETHING_H_
#define _DOSOMETHING_H_

#include <vector>

using namespace std;

typedef unsigned long long ULL;

typedef struct _MSG_STRUCT_
{
    ULL header : 16;
    ULL msg : 40;
    ULL time : 40;
    ULL body : 16;
    ULL tail : 21;
    ULL flag : 1;
    ULL continuous : 1;
} MSG_STRUCT;

class MSG_CLUSTER
{
public:
    vector<MSG_STRUCT> vect;

    void msg_show();
};

#endif

doSomeThing.cpp

#include <iostream>
#include <fstream>

#include "doSomeThing.h"

using namespace std;

void MSG_CLUSTER::msg_show()
{
    for (;;) 
    {
        if (!(vect.empty()))
        {
            vector<MSG_STRUCT>::iterator it_begin = vect.begin();

            while (it_begin != vect.end())
            {
                cout << hex << (*it_begin).header << endl;
                it_begin++;

                vect.erase(it_begin - 1);
            }
        }
    }
};

A few (unrelated) things: 1) All symbols beginning with an underscore and followed by an upper-case letter are reserved. See [What are the rules about using an underscore in a C++ identifier?](https://stackoverflow.com/questions/228783/what-are-the-rules-about-using-an-underscore-in-a-c-identifier) 2) All class and ***structure*** names are also type-names in C++, so you don't need `typedef` for structures. 3) [`using namespace std;` is a bad habit](https://stackoverflow.com/questions/1452721/why-is-using-namespace-std-considered-bad-practice), doing it in a header file doubly so. — Some programmer dude, Mar 08 '21 at 06:42
Also,, aliases like `ULL` doesn't make your code easier to read, understand and maintain, rather the opposite. All-uppercase names are almost universally used for preprocessor macros. And you don't need that `vect.empty()` condition, it's kind of implied in the iterator loop, which is commonly written as a `for` loop instead. And modifying the container you're iterating over is almost always a bad idea, I recommend you check what [the vector `erase` function](https://en.cppreference.com/w/cpp/container/vector/erase) *returns*. — Some programmer dude, Mar 08 '21 at 06:46
Unless using old compiler, you might prefer `it_begin = vect.erase(it_begin)` – apart from, it is pretty inefficient to remove from front again and again as all subsequent elements will be moved one position towards front with *every* erasure. Are you running the code in a multi-threaded scenario? Then your code is *not* thread-safe, you might be trying to read/erase from a vector the contents have been re-allocated in the meanwhile by another thread (RAV hints to exactly this). — Aconcagua, Mar 08 '21 at 07:08

Some programmer dude · Answer 1 · 2021-03-08T06:58:43.947

The problem is the way you iterate over and remove elements from the vector.

Think about when you come to the last element of the vector, you then do it_begin++ which will make it_begin == vect.end(). But then you erase the last element which changes the end, so it_begin will no longer be equal to vect.end() and the loop condition will never become false, and you will go out of bounds of the vector and have undefined behavior.

If you want to erase all elements as you iterate over them, you need to get a new iterator from the erase function:

cout << hex << it_begin->header << endl;
it_begin = vect.erase(it_begin);

And if you're doing this in a multi-threaded program, where other threads might add elements to the vector, you must protect the whole loop somehow, with e.g. a mutex or similar.

Thanks for your answer, I tried to correct the problem in this area, but the result still has such an error： Exception thrown: read access violation. — YC L, Mar 08 '21 at 07:21

Use vector in multithreading Exception thrown: read access violation

1 Answers1