[CXX-639] verifyFailed() in assert_util.cpp has undesired behavior in _DEBUG Created: 20/Jul/15 Updated: 06/Dec/16 Resolved: 27/Jul/15 |
|
| Status: | Closed |
| Project: | C++ Driver |
| Component/s: | Implementation |
| Affects Version/s: | legacy-1.0.0, legacy-1.0.3 |
| Fix Version/s: | legacy-1.0.4 |
| Type: | Bug | Priority: | Major - P3 |
| Reporter: | Gregory Vogel | Assignee: | Andrew Morrow (Inactive) |
| Resolution: | Done | Votes: | 0 |
| Labels: | legacy-cxx | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | Not Specified | ||
| Original Estimate: | Not Specified | ||
| Description |
|
There's #ifdef'ed code in aforementioned method that calls CRT's abort() function. There's a comment stating "this is so we notice in buildbot". The problem is, we don't get to notice anything in our code because the process exits. I don't understand why you'd do this and not allow client code to handle the exception that normally gets thrown in release code. For us, this is particularly problematic in automated testing, specifically with Google's GTest framework, which disables (aptly so), any message boxes displayed during abort(). The specific case this occurred for our team was in calls to various methods for BSONElement where the type of the element is verified. If there's a mismatch, boom, program is gone, which isn't what we want, ever. |
| Comments |
| Comment by Andrew Morrow (Inactive) [ 29/Jul/15 ] |
|
gregory.vogel@guidancesoftware.com - I understand. The change seems quite safe having thought about it for a few days, and it is passing all of our tests. I'm just going to release legacy-1.0.4 with this fix, as it seems very low risk. |
| Comment by Gregory Vogel [ 27/Jul/15 ] |
|
I might not be able to do this immediately. |
| Comment by Andrew Morrow (Inactive) [ 27/Jul/15 ] |
|
gregory.vogel@guidancesoftware.com I've just pushed the (obvious) fix for this. Would you mind re-testing in your development environment with the HEAD of the legacy branch, which has this fix, and let me know if it resolves it for you? This is the last open issue for legacy-1.0.4 after the rc0 release, and I'd like to release it as stable sometime soon. |
| Comment by Githook User [ 27/Jul/15 ] |
|
Author: {u'username': u'acmorrow', u'name': u'Andrew Morrow', u'email': u'acm@mongodb.com'}Message: |
| Comment by Andrew Morrow (Inactive) [ 27/Jul/15 ] |
| Comment by Gregory Vogel [ 21/Jul/15 ] |
|
That would be great. Thanks for considering fixing the issue. It would have been handy if it were simply unique #define beside _DEBUG. |
| Comment by Andrew Morrow (Inactive) [ 21/Jul/15 ] |
|
The code you are pointing to is indeed deeply flawed. While this is never the most satisfying answer, the answer to your question as to "why we would do this" unfortunately is "that is how it has always been". The origins of the legacy C++ driver are in the server codebase, and you can see that the same code exists in the server codebase to this day: The situation with the driver is actually a little worse though, since the controlling ifdef is _DEBUG, rather than a macro we control, which appears to have been an oversight in 16734cdd. I think the situation is severe enough that I will probably fix this for a legacy-1.0.4-rc1. Thanks for reporting it. |