std::map::find and std::map::end being weird

Question

So basically a few hours ago, I was working on my project and something that I didn't touch at all broke for no apparent reason. This code:

CFilesystem::working_directory_t &CFilesystem::GetThreadDirectories( )
{
    const auto dwThread = GetCurrentThreadId( );

    const auto pSearch = _ThreadDirectories.find( dwThread );
    volatile auto b = pSearch == _ThreadDirectories.end( );
    if ( pSearch == _ThreadDirectories.end( ) )
    {
        _ThreadDirectories.insert( std::pair< DWORD, working_directory_t >( dwThread, working_directory_t( ) ) );
        return GetThreadDirectories( );
    }

    return pSearch->second;
}

struct working_directory_t
{
    std::string strWorkingDirectory;
    std::stack< std::string > _DirectoryStack;

    working_directory_t( ) = default;
    void StoreCurrentWorkingDirectory( );
    void RestoreWorkingDirectory( );
};

std::map< DWORD, working_directory_t > _ThreadDirectories { };

doesn't want to work anymore. When this is run, the variables look like this: For whatever reason, the variable, b, evaluates to false, even though pSearch clearly equals end, as shown in the image. If I drag the yellow arrow into the if statement to execute the code that inserts into _ThreadDirectories, I get an exception thrown as follows:

I've tried rebuilding, restarting Visual Studio 2019 and my PC. I've tried inlining the CFileSystem declaration rather than externing it to see if that would make a difference, but it didn't. I have no idea what's happened and I'd appreciate any help. Thanks in advance.

Unrelated recommended reading: [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) — user4581301, Jun 29 '19 at 06:34
Are multiple threads accessing `_ThreadDirectories`? If so, is the access to it synchronized somehow? — VolkerG, Jun 29 '19 at 06:37
No, it's just one thread. _ThreadDirectories is empty when this is run. — , Jun 29 '19 at 06:38
Lot of potential unknowns here. What if everything was wrecked by dancing gremlins on line 67? — user4581301, Jun 29 '19 at 06:43
It's when it tries to access the search because it thinks that it was found. Basically every thread is supposed to have a certain directory open so the function just finds which directory a certain thread has open. — , Jun 29 '19 at 06:55
@ColeW Your question doesn't have enough information to be definitely answered. Post a [mcve] regarding the behavior as required here please. — πάντα ῥεῖ, Jun 29 '19 at 07:07
Why that recursive call? You could just have `search = insert().first;` and simply go on. Side note: if construction of the working directory object is cheaper than map lookup, you could just do the insert without checking before: `insert` only inserts, if the element doesn't exist yet. Second value of returned pair tells you if insertion actually took place. — Aconcagua, Jun 29 '19 at 07:22
[try_emplace](https://en.cppreference.com/w/cpp/container/map/try_emplace) seems to be the most interesting alternative, though. — Aconcagua, Jun 29 '19 at 07:26

jignatius · Accepted Answer · 2019-06-29T07:23:32.497

Firstly the function is calling itself if the thread id is not found. Can't you just return a reference to the value that is inserted?

Secondly, and most importantly, if multiple threads are accessing the same map it definitely needs a mutex to make it thread safe.

I would implement the function like this:

CFilesystem::working_directory_t &CFilesystem::GetThreadDirectories( )
{
    const auto dwThread = GetCurrentThreadId( );

    std::lock_guard<std::mutex> lock(mMutex);
    const auto pSearch = _ThreadDirectories.find( dwThread );

    if ( pSearch == _ThreadDirectories.end( ) )
    {
        _ThreadDirectories.insert( std::pair< DWORD, working_directory_t >( dwThread, working_directory_t( ) ) );
        return _ThreadDirectories[dwThread];
    }

    return pSearch->second;
}

where mMutex is a std::mutex object defined in CFilesystem.

I would also protect access to the map in other functions that use _ThreadDirectories. Do you have a function that deletes an entry? If so use a lock_guard there too.

You might have a look at [try_emplace](https://en.cppreference.com/w/cpp/container/map/try_emplace). You avoid double lookup and unnecessary object construction at the same time... — Aconcagua, Jun 29 '19 at 07:30

std::map::find and std::map::end being weird

1 Answers1