assertions: add new class of known issue assertions#10015
assertions: add new class of known issue assertions#10015mattklein123 merged 13 commits intoenvoyproxy:masterfrom
Conversation
|
Note this is a recreation of #9814 |
|
(In response to feedback from previous PR): I didn't create a RELEASE and regular version of this, since my line of thinking was that it represented a new class. e.g. 'normally' RELEASE_ASSERTS will always be present, and ASSERTS won't, this new class won't 'normally' be present even when regular ASSERTS are compiled in. |
mattklein123
left a comment
There was a problem hiding this comment.
Thanks for iterating on this. A few high level comments.
/wait
bazel/BUILD
Outdated
There was a problem hiding this comment.
Assuming we go with this change, I think this should be inverted. The default should still be for assertions to be enabled. If someone is hitting an issue and can't debug/fix they can disable the assertion.
There was a problem hiding this comment.
Sure, I'm fine with inverting it. Will update.
source/common/common/assert.h
Outdated
There was a problem hiding this comment.
Per above I think we should invert the logic. Still, I realize now that you are trying to disable a normal ASSERT vs. a RELEASE_ASSERT. Why are you shipping code with normal ASSERTs enabled? Are you using an optimized built with regular asserts on? Why not just turn them off?
There was a problem hiding this comment.
It has a lot to do with the early stage Envoy Mobile is in, and the fact that it's under active development. We're calling into Envoy via new mechanisms and in new contexts. Sometimes expectations fail to hold. Often it's because we've erred, and sometimes it's because we've uncovered a new edge case. Either way, at this point, we'd like to maintain that visibility by keeping (most) assertions compiled in. For now.
b784d12 to
51c6b30
Compare
|
Created this issue to reference in example usage: #10030 |
|
Please check format. /wait |
mattklein123
left a comment
There was a problem hiding this comment.
LGTM other than comment issue, thanks!
/wait
source/common/common/assert.h
Outdated
|
resuming local testing and will update comment |
Signed-off-by: Mike Schore <mike.schore@gmail.com>
Signed-off-by: Mike Schore <mike.schore@gmail.com>
Signed-off-by: Mike Schore <mike.schore@gmail.com>
Signed-off-by: Mike Schore <mike.schore@gmail.com>
Signed-off-by: Mike Schore <mike.schore@gmail.com>
Signed-off-by: Mike Schore <mike.schore@gmail.com>
Signed-off-by: Mike Schore <mike.schore@gmail.com>
Signed-off-by: Mike Schore <mike.schore@gmail.com>
Signed-off-by: Mike Schore <mike.schore@gmail.com>
d8af963 to
741c8f7
Compare
|
Friendly request to please never force push. It makes reviews much more difficult. Only merge master and add commits. Thank you! |
|
Release CI issue looks legit. PTAL. /wait |
Signed-off-by: Mike Schore <mike.schore@gmail.com>
Signed-off-by: Mike Schore <mike.schore@gmail.com>
Signed-off-by: Mike Schore <mike.schore@gmail.com>
Signed-off-by: Mike Schore <mike.schore@gmail.com>
|
I think the CI failure is maybe spurious now. |
Bumping to include envoyproxy/envoy#10015. Should resolve #572. Signed-off-by: Michael Rebello <me@michaelrebello.com>
Bumping to include envoyproxy/envoy#10015. Should resolve #572. Signed-off-by: Michael Rebello <me@michaelrebello.com>
Description: With this change envoyproxy/envoy#10015 Envoy now supports conditionally disabling assertions that cover known issues via build options. This allows us to resolve the crash on shutdown on iOS while the upstream issue is investigated. Risk Level: Low Testing: Local, upstream Signed-off-by: Mike Schore <mike.schore@gmail.com>
Bumping to include #10015. Should resolve envoyproxy/envoy-mobile#572. Signed-off-by: Michael Rebello <me@michaelrebello.com> Signed-off-by: JP Simard <jp@jpsim.com>
Description: With this change #10015 Envoy now supports conditionally disabling assertions that cover known issues via build options. This allows us to resolve the crash on shutdown on iOS while the upstream issue is investigated. Risk Level: Low Testing: Local, upstream Signed-off-by: Mike Schore <mike.schore@gmail.com> Signed-off-by: JP Simard <jp@jpsim.com>
Bumping to include #10015. Should resolve envoyproxy/envoy-mobile#572. Signed-off-by: Michael Rebello <me@michaelrebello.com> Signed-off-by: JP Simard <jp@jpsim.com>
Description: With this change #10015 Envoy now supports conditionally disabling assertions that cover known issues via build options. This allows us to resolve the crash on shutdown on iOS while the upstream issue is investigated. Risk Level: Low Testing: Local, upstream Signed-off-by: Mike Schore <mike.schore@gmail.com> Signed-off-by: JP Simard <jp@jpsim.com>
Description: Adds a new class of assertions, KNOWN_ISSUE_ASSERT, that may be compiled out even when other assertions are compiled in. The intent is to allow assertions to generally be compiled into releases, but to exclude instances that are known to fail for as-yet unknown or unresolved reasons. When the issue has been triaged and resolved, a KNOWN_ISSUE_ASSERT should generally become a regular ASSERT or RELEASE_ASSERT again.
Risk Level: Moderate
Testing: Built and ran locally with various combinations of build flags.
Signed-off-by: Mike Schore mike.schore@gmail.com