Deadlock in the setup of expectations for the mocked `GetCurrentThreadId` function about gmock-win32

[smalti]

I gave up on the “expectation clauses” step.

I experimented a little bit with wrappers around l-value references returned from EXPECT_CALL / ON_CALL and got some results with this test code:

#define MOCK_MODULE_FUNC0_(tn, constness, ct, func, ...) \
std::unique_ptr< ::testing::NiceMock< struct mock_module_##func > > func##obj; \
...
template< typename Reference >
struct ref_proxy
{
    ~ref_proxy() { use_module_mock = true; }
    operator Reference() const { return r; }

public:
    Reference r;
};

template< typename Reference >
ref_proxy< Reference > makeProxy(Reference&& r)
{
    return ref_proxy< Reference >{ std::forward< decltype(r) >(r) };
}
...
extern thread_local volatile bool use_module_mock;
...
#define EXPECT_MODULE_FUNC_CALL(func, ...) \
    if (!func##obj) \
    { \
        func##obj.reset(new ::testing::NiceMock< struct mock_module_##func >{ }); \
    } \
    if (!mock_module_##func::oldFn_) \
    { \
        mockModule_patchModuleFunc(&func, reinterpret_cast< void* >( \
            &mock_module_##func::stub), &mock_module_##func::oldFn_); \
    } \
    use_module_mock = false; \
    static_cast< decltype(EXPECT_CALL(mock_module_##func::instance(), \
        func(__VA_ARGS__)))& >(makeProxy(EXPECT_CALL(mock_module_##func::instance(), func(__VA_ARGS__))))

Then we could hold a lock until the clause expression invocation finished (then it will be automatically unlocked):

TEST(GetCurrentThreadIdTest, BaseTest)
{
    ON_MODULE_FUNC_CALL(GetCurrentThreadId).WillByDefault(Return(42U));
    
    // In this case, the lock will be unlocked immediately after the Times(...) clause was called
    EXPECT_MODULE_FUNC_CALL(GetCurrentThreadId).Times(2);

    const auto tid1 = GetCurrentThreadId();
    const auto tid2 = GetCurrentThreadId();
    ...

I'm not sure yet about the reliability of this proxy-based approach to achieve the desired outcome, but it works in that test code.

[Alex0vSky]

Calls to GetCurrentThreadId come from the testing::internal::GTest Mutex Lock class.
You can wrap all thread-safe calls so you know when to call the actual function... this is very difficult for gmock+gtest.

Option 1: disable using GetCurrentThreadId from testing::internal::GTestMutexLock

#pragma comment( lib, "kernel32" )

// Disable using mutex
#define GTEST_IS_THREADSAFE 0
	// trick, due 
	// error C2065: 'AutoHandle': undeclared identifier
	// @issue https://github.com/google/googletest/issues/3586
	// @issue https://github.com/google/googletest/issues/4305
#include "..\..\gmock\include\gtest\internal\gtest-port.h"
#undef GTEST_HAS_DEATH_TEST

#include "..\src\gtest\gtest-all.cc"
#include "..\src\gmock\gmock-all.cc"
#include "..\..\src\gmock-win32.cpp"

using namespace testing;
	
MOCK_STDCALL_FUNC(DWORD, GetCurrentThreadId);

int main(int argc, char* argv[])
{
    ON_MODULE_FUNC_CALL(GetCurrentThreadId).WillByDefault(Return(1));
    return 0;
}

Option 2: disable using GetCurrentThreadId for gmock-win32
in compile-time

// C++11
constexpr bool strings_equal(char const * a, char const * b) {
    return *a == *b && (*a == '\0' || strings_equal(a + 1, b + 1));
}
...
#define ON_MODULE_FUNC_CALL(func, ...) \
    static_assert(!strings_equal(#func, "GetCurrentThreadId" ), \
    "it is prohibited to use the function GetCurrentThreadId"); \
    if (!mock_module_##func::oldFn_) \
    { \
        mockModule_patchModuleFunc(&func, reinterpret_cast< void* >( \
            &mock_module_##func::stub), &mock_module_##func::oldFn_); \
    } \
    ON_CALL(mock_module_##func::instance(), func(__VA_ARGS__))

[smalti]

Option 2: disable using GetCurrentThreadId for gmock-win32
in compile-time

Yes, good idea, and there are several other (inside GTest) Win32 API functions that may interact with client tests through mocks:

CloseHandle
InitializeCriticalSection
DeleteCriticalSection
EnterCriticalSection
LeaveCriticalSection
InterlockedCompareExchange
Sleep
CreateThread
WaitForSingleObject
GetLastError
SetThreadPriority
GetThreadPriority
GetCurrentThread
ResumeThread
CloseHandle
GetTempPathA
GetTempFileNameA
etc.

This list may change in the next version of the GTest library. Therefore, it might be advisable to provide some recommendations about avoiding such function mocks in tests.

---

You can wrap all thread-safe calls so you know when to call the actual function.

I agree; that could be also a solution. It is not a simple one, but I will try to implement it by creating a barrier-like restoring to the original function during GMock internal calls. We can achieve this by modifying our stub function (along with other macro definitions) as shown in the following pseudocode:

struct mock_module_##func
{
...
    static GMOCK_RESULT_(tn, __VA_ARGS__) ct stub(...)
    {
        const bool invokeOrig = lock > 0;
        
        // Increment lock for the current thread
        const LockBarrier barrier{ lock };

        // The subsequent reentrant calls will be directed to the original function. Consequently, all
        // GMock operations will call real APIs during this active barrier
        
        if (invokeOrig)
            return invokeOrig(mock_module_##func::oldFn_, ...);
        else
            return mock_module_##func::instance().func(...);
    }
};
...
// Locks per thread
extern thread_local int lock;

We must implement these locks for all GMock functions invoked within our macro definitions and for expectation clauses as well. Additionally, we should introduce a macro to temporarily suppress API mocks during the execution of other GTest macro-defines. This macro can wrap around GTest macro definitions, and we can also implement scoped suppression of API mocks, e.g.:

EXPECT_MODULE_FUNC_CALL(GetCurrentThreadId).Times(2);
...
// Used to avoid GetCurrentThreadId mocks during EXPECT_EQ check (because
// we could get over-saturated results or even deadlock)
BYPASS_MOCKS(EXPECT_EQ(tid, 42U));
...
{
    const BypasMocks useOrigAPI{ };
    // Some other code or GTest ASSERTs that shouldn't be involved in the testing of APIs
    ...
}

[Alex0vSky]

provide some recommendations about avoiding such function mocks in tests.

Yes, after wrapping the WinApi function used in the gmock-win32 core. There are few of them: GetLastError, GetCurrentProcess, WriteProcessMemory, VirtualProtect and GetModuleHandle. It will be easy.
The complete list of gmock/gtest imports can be obtained during the CI testing phase from a dummy exe file using the Msvc Dumpbin tool or the Python tool. When a new version of "googletest" comes out, we can run it and compare it with the existing ones.
I don't know how this will affect compilation performance -- if the list is very large.

We can achieve this by modifying our stub function

I tried it, it works. There are "sync" call in SetOwnerAndName() and RegisterOwner() too. But... yes!

all GMock functions invoked within our macro definitions and for expectation clauses

i gave up on the “expectation clauses” step.

All GMock functions... how many are there?
I agree, this is the only way I see so far.

And one more task

ON_CALL(mock_module_GetCurrentThreadId::instance(), GetCurrentThreadId())
			.WillByDefault( testing::Return( 1 ) );

if for example1 in ON_MODULE_FUNC_CALL

#define ON_MODULE_FUNC_CALL(func, ...) \
...
mock_module_GetCurrentThreadId::invokeOrig = true; \
auto tmp = ON_CALL(mock_module_GetCurrentThreadId::instance(), GetCurrentThreadId()); \
mock_module_GetCurrentThreadId::invokeOrig = false; \
tmp
...
ON_MODULE_FUNC_CALL( GetCurrentThreadId )
    .WillByDefault( testing::Return( 1 ) );

get warning about WillByDefault, it can detect temporary object or something.

if for example2 in ON_MODULE_FUNC_CALL

#define ON_MODULE_FUNC_CALL(func, ...) \
...
struct setter { \
 ~setter() { \
  mock_module_GetCurrentThreadId::invokeOrig = false; \
 } \
}; \
auto y = [](){  \
  mock_module_GetCurrentThreadId::invokeOrig = true; \
  setter selfRemove; \
  return ON_CALL(mock_module_GetCurrentThreadId::instance(), GetCurrentThreadId()); \
}; y( )
...
ON_MODULE_FUNC_CALL( GetCurrentThreadId )
    .WillByDefault( testing::Return( 1 ) );

Even RVO doesnt help, warning about WillByDefault

[Alex0vSky]

Cool!
It was an "l-value reference". I should have looked deeper.
Everything means that it can be solved, this gives us optimism that everything will work out soon. After all, there are actually only two macros - EXPECT_MODULE_FUNC_CALL and ON_MODULE_FUNC_CALL to wrap EXPECT_CALL and ON_CALL.
Suddenly.

[smalti]

Yes, after wrapping the WinApi function used in the gmock-win32 core.

Good point, I forgot about the APIs used in this library, yes, we should also wrap them.

Everything means that it can be solved, this gives us optimism that everything will work out soon.

The one thing that's bothering me is this macro:

BYPASS_MOCKS(EXPECT_EQ(tid, 42U));

I don't know how we can avoid such wrapping by BYPASS_MOCKS (it does make the test code somewhat more complex).

[Alex0vSky]

Replacing macro EXPECT_EQ?
Not an easy decision.
Here's how I can help #7