[SERVER-43256] Fix incorrect uses of assert.soon and make hang-analysis call exit Created: 10/Sep/19 Updated: 29/Oct/23 Resolved: 31/Oct/19 |
|
| Status: | Closed |
| Project: | Core Server |
| Component/s: | Internal Code |
| Affects Version/s: | Backlog |
| Fix Version/s: | 4.3.1 |
| Type: | New Feature | Priority: | Major - P3 |
| Reporter: | Ryan Timmons | Assignee: | Robert Guo (Inactive) |
| Resolution: | Fixed | Votes: | 0 |
| Labels: | None | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | Not Specified | ||
| Original Estimate: | Not Specified | ||
| Issue Links: |
|
||||||||
| Backwards Compatibility: | Fully Compatible | ||||||||
| Sprint: | STM 2019-10-31 | ||||||||
| Participants: | |||||||||
| Story Points: | 3 | ||||||||
| Description |
|
A number of places use assert.soon as a retry mechanism (e.g. ssl_test.js), and at least one test (index_delete.js) runs assert.soon in a try/finally block to capture better error messages. ↑ looks like this:
In the cases where assert.soon fails due to timeout, we want to instead run hang-analysis and exit rather than returning control to the caller. (Comment from Robert: I think assert.soon almost always fails due to a timeout. What's the reason for exiting in this case?) Uses of assert.soon are pervasive in jstests so do a best-effort fix here. Could modify assert.soon in a patch-build to throw immediately and any test that doesn't fail is likely using it incorrectly. If there are more than (say) 3-4 cases of using assert.soon as a retry mechanism, create a helper assert.retry method (name/location tbd) that has a similar signature to assert.soon but doesn't call hang-analyzer or barf if the callback is never truthy and instead returns a [success, error] array where success is the last result of the callback and error is any errors thrown by the callback. (Exact signature tbd depending on how it's used by callers of extant assert.soon.) Finally, once this is done, modify existing callers of the hang-analyzer (probably just assert.soon) to call exit after running hang-analysis. |
| Comments |
| Comment by Githook User [ 31/Oct/19 ] |
|
Author: {'name': 'Robert Guo', 'email': 'robert.guo@mongodb.com'}Message: |
| Comment by Ryan Timmons [ 25/Oct/19 ] |
|
Be suspicious any time there's an assert inside of an assert.throws e.g. here |
| Comment by Ryan Timmons [ 24/Oct/19 ] |
|
A patch-test of calling hang-analyzer from assert.js is here. Most of the reds are a result of the code in my last comment. |
| Comment by Ryan Timmons [ 24/Oct/19 ] |
|
One particularly obvious case of using assert.soon incorrectly is here: |
| Comment by Max Hirschhorn [ 11/Sep/19 ] |
I would think we'd still want the mongo shell to log the JavaScript stacktrace along with the "failed to load" message picked up by the Build Baron tooling of where the exception from assert.soon() came from. |