Why am I getting duplicate values in a C++ thread function call?

Question

I'm curious to what I'm doing wrong here. I have the following function:

indexer.h:

class DtIndexer {
public:
    static void ThreadedIndex(string folderPath);

indexer.cpp

void DtIndexer::ThreadedIndex(string folderPath) {
    cout << "\t-> Indexing Folder: " << folderPath << endl;
    cout << "\t->> Done..." << endl;
}

and my call to create the threads:

void DtIndexer::UpdateIndex(DatabaseData &data, bool isCreate) {
    vector<thread> threadList;

    for (string &s: data.FilePaths) {
        const char *folder = GetFolderPath(s, data.IncludeSubFolders);
        cout << "\t-> Adding Folder to Thread: " << folder << endl;
        threadList.emplace_back(thread(ThreadedIndex, folder));
    }

    for_each(threadList.begin(), threadList.end(), mem_fn(&thread::join));
}

My output is this:

-> Adding Folder to Thread: /index_2185<+>

-> Adding Folder to Thread: /index_1065<+>

-> Indexing Folder: /index_1065<+>

->> Done...

-> Indexing Folder: /index_1065<+>

->> Done...

Now, I'm pretty sure it has to deal with the static for the method, but if I remove static I get this:

error: invalid use of non-static member function ‘void DtIndexer::ThreadedIndex(std::__cxx11::string)’ threadList.emplace_back(thread(ThreadedIndex, folder));

Also, If I remove static and add the function to the thread like this:

threadList.emplace_back(thread(&DtIndexer::ThreadedIndex, folder));

I get:

required from here /usr/include/c++/6/functional:1286:7: error: static assertion failed: Wrong number of arguments for pointer-to-member static_assert(_Varargs::value

I'm still fairly new to C++, so, any advice will be appreciated.

Answer 1

Replace

const char *folder = GetFolderPath(s, data.IncludeSubFolders);

with

std::string folder( GetFolderPath(s, data.IncludeSubFolders) );

The thread starts by copying the arguments by value and you copy a const char* which probably points to a static buffer in the GetFolderPath function. That buffer will be overwritten for each call you do so the threads that is reading from it may or may not get what you expect.

Note that you don't need to do emplace_back(std::thread(... since what you write as arguments to emplace_back is forwarded to the constructor of the type your vector is holding, which in this case is std::thread. You also do not need to make the thread function static unless you really want to. You could start with a lambda, capturing this and folder by copy. Another thing you may notice is that the output gets garbled when many threads write to std::cout at the same time. You can guard common resources (like std::cout) against simultaneous accesses by multiple threads using std::mutex and std::lock_guard.

Here's a version of your class with a non-static thread method and a guard for std::cout:

#include <iostream>
#include <vector>
#include <cstring>
#include <mutex>
#include <thread>

const char* GetFolderPath() {
    static std::vector<std::string> paths = {"this is a path", "here is another one",
                                             "what do we have here", "mother of dirs",
                                             "what did the tomatoe ..."};
    static size_t idx = 0;
    static char buf[256];
    std::strcpy(buf, paths[idx].c_str());
    idx = (idx + 1) % paths.size();
    return buf;
}

class DtIndexer {
    std::mutex mtx_cout; // std::cout mutex

public:
    void ThreadedIndex(std::string folderPath) {
        std::lock_guard lock(mtx_cout); // wait until this thread acquires the mutex
        std::cout << "\t-> Indexing Folder: " << folderPath << "\n";
    }

    void UpdateIndex() {
        std::vector<std::thread> threadList;

        { // lock_guard scope
            std::lock_guard lock(mtx_cout); // acquire mutex

            for(int i = 0; i < 50; ++i) {
                // const char* folder = GetFolderPath();
                std::string folder(GetFolderPath());

                std::cout << "\t-> Adding Folder to Thread: " << folder << "\n";

                //             safe copy here --+       +--- thread runs here ---+
                //                              |       |                        |
                //                              V       V                        V
                threadList.emplace_back( [this, folder] { ThreadedIndex(folder); });
                //                      Δ
                //                      |
                //                      +-- no std::thread(... needed here
            }
        } // lock_guard releases mutex here

        for(auto& t : threadList) t.join();
    }
};

int main() {
    DtIndexer a;
    a.UpdateIndex();
}

You can replace the folder string above with a const char* and see the same problem you got.