Sanity checking libevent is initialized#2339
Conversation
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
ggreenway
left a comment
ggreenway
left a comment
There was a problem hiding this comment.
I think the bool should be moved to a class static, the accessor put into the header, and the ASSERT changed to RELEASE_ASSERT (since it will be basically free if the accessor is inline).
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
|
Sure! I'd only used ASSERT I figured since it is way more likely to affect Googlers (using our internal version of gtest) and this would minimize the impact on everyone else. |
|
I think I'm in the minority, but I prefer all ASSERTs to be RELEASE_ASSERT unless it has a large performance impact (like if a bunch of code had to be run to calculate the thing that is being asserted). I'd rather have it abort than run in an unexpected/unsupported state. |
| post_timer_(createTimer([this]() -> void { runPostCallbacks(); })), | ||
| current_to_delete_(&to_delete_1_) {} | ||
| current_to_delete_(&to_delete_1_) { | ||
| ASSERT(Libevent::Global::initialized()); |
There was a problem hiding this comment.
Oh, this one should probably also be RELEASE_ASSERT
There was a problem hiding this comment.
What are you talking about, I already changed that file! ... facepalm :-P
| class Global { | ||
| public: | ||
| // True if initialized() has been called. | ||
| static bool INITIALIZED; |
There was a problem hiding this comment.
I think this should be lower case, this isn't a constant?
There was a problem hiding this comment.
nit: This variable can be private.
There was a problem hiding this comment.
Oh, is the CAPS only for contants? I thought it was for all static.
Ignoring the naming conflict, static bool intitialized; is weird and static bool initialized_ isn't correct.
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
|
|
||
| private: | ||
| // True if initialized() has been called. | ||
| static bool is_initialized; |
There was a problem hiding this comment.
I think is_initialized_ or initialized_ is the correct name, even though it is static:
https://google.github.io/styleguide/cppguide.html#Variable_Names section "Class Data Members", and I didn't see an override in STYLE.md
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
This reverts commit 38a2475. Signed-off-by: Piotr Sikora <piotrsikora@google.com>
Signed-off-by: GitHub Action <noreply@github.com> Co-authored-by: jpsim <jpsim@users.noreply.github.com> Signed-off-by: JP Simard <jp@jpsim.com>
Signed-off-by: GitHub Action <noreply@github.com> Co-authored-by: jpsim <jpsim@users.noreply.github.com> Signed-off-by: JP Simard <jp@jpsim.com>
4 participants
We've had a couple of instances of internal Envoy with custom main or custom tests failing to initialize things properly and causing hard to debug timeouts / tsan warnings.
Adding RELEASE_ASSERTs to hopefully catch this earlier and in a more obvious way.
Risk Level: Low
Testing: n/a
Release Notes: n/a