Don't throw nested exception from dtor of Mock object #1098
Conversation
There is a problem happened when GMock is used with 3rdParty UT-engine based on exceptions handling. If I enables throwing exceptions from GMock: ::testing::GTEST_FLAG(throw_on_failure) = true Then in test case if I created a local Mock object with invalid expectation call then I will get exception in exception: Because of invalid arguments passed to the method. During stack unwinding we will destroy Mock object and will call VerifyAndClearExpectationsLocked(); from~FunctionMockerBase() These behavior obviously leads to process termination. The suggestion is to check whether we already in unhandled exception or not before Verifing expectations in destructor.
|
@BillyDonahue could you please review these few lines. |
|
@rgolovanov, he merges google's internal changes only, don't panic |
| GTEST_LOCK_EXCLUDED_(g_gmock_mutex) { | ||
| MutexLock l(&g_gmock_mutex); | ||
| VerifyAndClearExpectationsLocked(); | ||
| if(!std::uncaught_exception()) |
There was a problem hiding this comment.
This is deprecated in C++17. I wonder if that will affect us.
There was a problem hiding this comment.
Sorry, it is not truth. In C++17 the return type is changed only:
cppreference.com
bool uncaught_exception();
(1) (deprecated in C++17)
int uncaught_exceptions();
(2) (since C++17)
(2) Detects how many exceptions in the current thread have been thrown or rethrown and not yet entered their matching catch clauses.
So the proposed change is correct.
There was a problem hiding this comment.
name changed: uncaught_exceptionS
There was a problem hiding this comment.
oh, yep. I missed it first time. Should I modify this fix using GTEST_GCC_VER_ to support both versions of C++?
There was a problem hiding this comment.
imho it's better to use __cplusplus macro(std, §16.8)
std::uncaught_exception has been modified to std::uncaught_exceptions in c++17, therefore we have to split fix for c++ versions before and after c++17
|
@BillyDonahue, @lor-anonymous-spirit could you please advice somebody who can review and accept this pull request. |
|
As a general rule of thumb, you shouldn't rely on std::uncaught_exceptions(). |
| #else | ||
| const bool inside_uncaught_exception = std::uncaught_exceptions() > 0; | ||
| #endif | ||
| if(!inside_uncaught_exception) |
There was a problem hiding this comment.
Can you explain why it's okay to not VerifyAndClearExpectationsLocked if we're inside an unhandled exception? I'm concerned that this conditional evaluation will make an invariant more complex.
| MutexLock l(&g_gmock_mutex); | ||
| VerifyAndClearExpectationsLocked(); | ||
|
|
||
| #if __cplusplus < 201703L |
There was a problem hiding this comment.
where is this magic number from? I can't find a settled numeric code for C++17.
| #if __cplusplus < 201703L | ||
| const bool inside_uncaught_exception = std::uncaught_exception(); | ||
| #else | ||
| const bool inside_uncaught_exception = std::uncaught_exceptions() > 0; |
There was a problem hiding this comment.
would be more elegant as a function than as a variable.
| const bool inside_uncaught_exception = std::uncaught_exceptions() > 0; | ||
| #endif | ||
| if(!inside_uncaught_exception) | ||
| VerifyAndClearExpectationsLocked(); |
| #if __cplusplus < 201703L | ||
| const bool inside_uncaught_exception = std::uncaught_exception(); | ||
| #else | ||
| const bool inside_uncaught_exception = std::uncaught_exceptions() > 0; |
There was a problem hiding this comment.
This isn't really the intended use of std::uncaught_exceptions(). To use std::uncaught_exceptions() appropriately, you should capture the number of uncaught exceptions on object construction and check again at destruction. If the number at destruction is different (it should always differ by 0 or 1) then you know you can't throw.
See the proposal.
Also worth mentioning, in the proposal is implementations for std::uncaught_exceptions() for older versions of MSVC and gcc/clang. Perhaps googletest should attempt to use those instead of defaulting to std::uncaught_exception.
See this github repo
|
@rgolovanov Sincere apologies for the long wait. I see unfinished discussion. If this i still relevant could you please rebase and we can try to move forward from here/ |
6 participants
fix of #1097
There is a problem happened when GMock is used with 3rdParty UT-engine based on exceptions handling.
If I enables throwing exceptions from
GMock: ::testing::GTEST_FLAG(throw_on_failure) = trueand if I created a local Mock object with invalid expectation call then I will get exception in exception:
VerifyAndClearExpectationsLocked();from~FunctionMockerBase()These behavior obviously leads to process termination.
The suggestion is to check whether we already in unhandled exception or not before Verifing expectations in destructor.