[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: CXX-639 Don't make assert behavior build type dependent
Branch: legacy
https://github.com/mongodb/mongo-cxx-driver/commit/592eddc59ddda82c248a615505c67a7c61e1dcc0

Comment by Andrew Morrow (Inactive) [ 27/Jul/15 ]

https://github.com/mongodb/mongo-cxx-driver/pull/301

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:

https://github.com/mongodb/mongo/blob/d1aca9594cdd10d56720b4cac28cef7a6eefa2da/src/mongo/util/assert_util.cpp#L142

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.

Generated at Wed Feb 07 21:59:49 UTC 2024 using Jira 9.7.1#970001-sha1:2222b88b221c4928ef0de3161136cc90c8356a66.