Add quic_thread_impl.h to QUICHE platform implementation.#6174
Add quic_thread_impl.h to QUICHE platform implementation.#6174alyssawilk merged 3 commits intoenvoyproxy:masterfrom
Conversation
Signed-off-by: Bin Wu <wub@google.com>
|
/assign @mpwarres |
|
|
||
| virtual ~QuicThreadImpl() { | ||
| if (thread_ != nullptr) { | ||
| PANIC("QuicThread should be joined before destruction."); |
There was a problem hiding this comment.
PANIC might be too severe: google3 logs at ERROR and Chromium DCHECKs for this. How about just logging at ERROR?
There was a problem hiding this comment.
There is a subtle problem, if the QuicThread is destructed immediately after Start(), but before the Run() function is called, then QuicThread::Run() becomes a call to a pure virtual function(since child has destructed), which aborts the program.
I can change QuicThread::Run() to a empty function to avoid the abort. How's that sound?
There was a problem hiding this comment.
I see. On second thought, allowing the QuicThreadImpl's Run() method to execute after/while the instance has been destructed seems like inviting problems. Making QuicThread::Run() an empty function feels like skating around the issue. Let's keep the PANIC as-is.
(Another option would be to do something more complicated, like having an internal enum to differentiate between the states of "not started" vs "started but Run not invoked yet" vs "Run invoked", but that feels like overkill for behavior that we shouldn't be triggering or need, anyways. I prefer what you have currently.)
…l_thread Signed-off-by: Bin Wu <wub@google.com>
|
|
||
| virtual ~QuicThreadImpl() { | ||
| if (thread_ != nullptr) { | ||
| PANIC("QuicThread should be joined before destruction."); |
There was a problem hiding this comment.
I see. On second thought, allowing the QuicThreadImpl's Run() method to execute after/while the instance has been destructed seems like inviting problems. Making QuicThread::Run() an empty function feels like skating around the issue. Let's keep the PANIC as-is.
(Another option would be to do something more complicated, like having an internal enum to differentiate between the states of "not started" vs "started but Run not invoked yet" vs "Run invoked", but that feels like overkill for behavior that we shouldn't be triggering or need, anyways. I prefer what you have currently.)
Signed-off-by: Bin Wu <wub@google.com>
Description:
Add quic_thread_impl.h to QUICHE platform implementation.
QuicThreadImpl is implemented on top of Envoy::Thread. It is used by some tests and utility programs in QUICHE, but not in core QUICHE code.
Risk Level: minimal: code not used yet
Testing:
bazel test test/extensions/quic_listeners/quiche/platform:quic_platform_test --test_output=all --define quiche=enabled --experimental_remap_main_repo
bazel test test/extensions/quic_listeners/quiche/platform:quic_platform_test --test_output=all --define quiche=enabled --experimental_remap_main_repo -c opt
Docs Changes: none
Release Notes: none
[Optional Fixes #Issue]
[Optional Deprecated:]