[SERVER-6317] Make assertions behave uniformly in debug and normal builds Created: 05/Jul/12  Updated: 28/Aug/23

Status: Backlog
Project: Core Server
Component/s: Testing Infrastructure
Affects Version/s: 2.1.2
Fix Version/s: None

Type: Task Priority: Major - P3
Reporter: Randolph Tan Assignee: Backlog - Service Architecture
Resolution: Unresolved Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Issue Links:
Depends
is depended on by SERVER-6315 tagSet suite test failure on debug bu... Closed
Duplicate
is duplicated by SERVER-20882 remove the verify() macro Closed
Assigned Teams:
Service Arch
Participants:

 Description   

Issue is that, since we abort on some of the assertion failure (only in DEBUG build), tests that tries to check whether an assertion is activated will fail because it aborts the mongo process instead of just throwing the AssertionException.



 Comments   
Comment by Steven Vannelli [ 10/May/22 ]

Moving this ticket to the Backlog and removing the "Backlog" fixVersion as per our latest policy for using fixVersions.

Comment by Andrew Morrow (Inactive) [ 20/Jun/18 ]

Thanks max.hirschhorn - sent it over to your team, still in needs triage.

Comment by Max Hirschhorn [ 20/Jun/18 ]

I think the TIG team can take it and send the Platforms team a code review.

Comment by Andrew Morrow (Inactive) [ 20/Jun/18 ]

max.hirschhorn - I'm fine with that approach. Are you hoping that platforms would implement that, or is that within TIGs scope of activity, in which case I'm sure platforms is willing to help with review.

Comment by Max Hirschhorn [ 20/Jun/18 ]

I think we should add -DVERIFY_IS_INVARIANT and try to stand up a new build variant in Evergreen. We can probably stage it incrementally with what Evergreen tasks we run on it. For instance, we currently don't run the jstestfuzz* Evergreen tasks against DEBUG builds because we'd otherwise hit verify() failures.

Comment by Andrew Morrow (Inactive) [ 19/Jun/18 ]

max.hirschhorn - I've moved this to 'Needs Triage' so we can discuss in our team triage meeting, but I'm sort of the opinion that we should kick this back to TIG to decide which way they want this resolved - I've shared my thoughts on the options. We can also decide to just keep on doing what we have been doing for 6 years and just close this as Won't Fix.

Comment by Andrew Morrow (Inactive) [ 14/Jun/18 ]

I don't like verify because when you see one in the code you can't know what was originally meant. Did the original author really mean invariant, meaning that the impossible happened, but reached for the wrong tool and wrote verify? Or did they mean dassert but were too scared to continue in release mode, and therefore should have written something stronger? Or did they mean uassert, in that the operation should fail, which is what happens in release mode, but they wanted tests to die? The whole thing is a mess. I think we need to commit to bulk converting all verify calls to either invariant or uassert. The former choice is the aggressive one (user reachable verify calls will now become crashes), and would destabilize things until we found the places where verify was really supposed to mean uassert. Moving verify to uassert is the conservative choice, in that there will be no new unintended crashing paths. I think the decision should lie with the TIG team as to which way to play it. Either way, we could definitely take your suggestion of standing up a build that enforces that choice, via a -DVERIFY_IS_INVARIANT or -DVERIFY_IS_UASSERT. Or do both and get them both green?

Comment by Max Hirschhorn [ 26/May/18 ]

acm, what would be your recommendation to convert existing verify() calls to the appropriate invariant() or uassert() calls?

Generated at Thu Feb 08 03:11:20 UTC 2024 using Jira 9.7.1#970001-sha1:2222b88b221c4928ef0de3161136cc90c8356a66.