google / googletest

GoogleTest - Google Testing and Mocking Framework
https://google.github.io/googletest/
BSD 3-Clause "New" or "Revised" License
34.79k stars 10.15k forks source link

[Bug]: Mutex deadlock when expecting a function call #4560

Open James-Tyne opened 5 months ago

James-Tyne commented 5 months ago

Describe the issue

A test started freezing after a code change. I traced it to a recursive attempt to lock a mutex (and simplified the code to reproduce).

Picture this scenario:
There is a class that holds a pointer to an interface. Equality between objects of this class is defined in part by data obtained from this interface, such as getting an ID number of the interface implementation. There is also a second class that interacts with the first class. More precisely, the second class has a member function whose parameter is the first class.

Now picture a test suite for the second class. Since the first class is "just" support, it gets mocked in these tests. More precisely, the interface gets mocked. One of the tests in the suite expects that a certain mock will be provided to the member function as a parameter. No special matchers, just simple equality. However, this hangs when the function is called.

Steps to reproduce the problem

#include <gmock/gmock.h>
#include <gtest/gtest.h>
#include <functional>

/**
 * The first ingredient is a type that has a mockable member and whose
 * equality operator depends on calling this member.
 */
struct Parameter {
  // Instead of a pointer to an interface, this class has a `std::function`.
  // It's the same principle, just simpler to write.
  std::function<int()> toMock;

  friend bool operator==(const Parameter& lhs, const Parameter& rhs) {
    return lhs.toMock() == rhs.toMock();
  }
};

/**
 * The second ingredient is a mocked function that takes the first ingredient
 * as a parameter.
 */
struct MockInterface {
  MOCK_METHOD(void, foo, (Parameter));
};

/**
 * We combine the ingredients in a test that expects the function of the
 * second ingredient to be called with a specific object of the first
 * ingredient.
 */
TEST(GTest, Deadlock) {
  // Set up the mock
  testing::MockFunction<int()> mock;
  ON_CALL(mock, Call()).WillByDefault(testing::Return(2));

  // Set up the parameter
  Parameter data{.toMock = mock.AsStdFunction()};

  // Expect the mock to be called with the parameter
  MockInterface test;
  EXPECT_CALL(test, foo(data)).Times(1);

  // Invoke the function.
  //
  // 1. `FindMatchingExpectationLocked()` is called as part of finding a
  //    matching expectation. This function locks the `g_gmock_mutex` mutex.
  //
  // 2. `operator==` is used to compare the given argument to what was
  //    specified in `EXPECT_CALL`.
  //
  // 3. `toMock` is invoked, which redirects to the mocked function.
  //
  // 4. `SetOwnerAndName` is called as part of invoking the mocked function.
  //    This function also locks the `g_gmock_mutex` mutex. Well, tries to.
  //    Deadlock!
  test.foo(data);
}

What version of GoogleTest are you using?

1.14.0

What operating system and version are you using?

Linux

What compiler and version are you using?

gcc version 11.4.1

What build system are you using?

cmake version 3.26.5

Additional context

No response

James-Tyne commented 4 months ago

It would be awkward for my real setup, but I could see an argument that a fake be used instead of a mock in this case. (It's awkward for me because that would mean splitting the interface; part of my interface is used in equality checking while another part is under test.) If the official stance is that a mock should not be used in this way, please consider this a feature request for better feedback from the test. With this indirect setup, it is not easy to see why a test deadlocks. However, it should not be hard for GoogleTest to detect this situation and emit a test failure instead of freezing.