Skip to content

Fix construction order of ServiceImpl and RequestSourceServiceImpl.#877

Merged
mum4k merged 2 commits intoenvoyproxy:mainfrom
mum4k:test_gcc_fix
Jul 18, 2022
Merged

Fix construction order of ServiceImpl and RequestSourceServiceImpl.#877
mum4k merged 2 commits intoenvoyproxy:mainfrom
mum4k:test_gcc_fix

Conversation

@mum4k
Copy link
Collaborator

@mum4k mum4k commented Jul 16, 2022

Also re-enable test_gcc.

Fixes #872.

Details:

  • ServiceImpl creates Envoy::Logger::Context which requires Envoy::Thread::MutexBasicLockable.
  • The Envoy::Thread::MutexBasicLockable was constructed after the Envoy::Logger::Context, so Envoy::Logger::Context became invalid during destruction of ServiceImpl.
  • this PR ensures that Envoy::Thread::MutexBasicLockable lives longer than Envoy::Logger::Context.
  • Fixing the same problem in the construction of RequestSourceServiceImpl.

Reason for test_gcc failure:

  • Only call ares_library_init()/cleanup() routines in process_wide() if ares extension used in DNS resolving. envoy#21884 triggered this bug, because it added a logging call initiated indirectly from the destructor of Envoy::ProcessWide.
  • This logging call happened during the destruction of ServiceImpl when the Envoy::Thread::MutexBasicLockable instance was already destroyed.
  • As a result the PURE virtual methods on its parent object Envoy::Thread::BasicLockable were called.
  • test_gcc reported it because it places a special function meant to detect this behavior in the vtbl of unimplemented pure virtual methods.

Signed-off-by: Jakub Sobon mumak@google.com

Signed-off-by: Jakub Sobon <mumak@google.com>
@mum4k mum4k changed the title Fix construction order of Envoy::Logger::Context. Fix construction order of ServiceImpl. Jul 16, 2022
Signed-off-by: Jakub Sobon <mumak@google.com>
@mum4k mum4k changed the title Fix construction order of ServiceImpl. Fix construction order of ServiceImpl and RequestSourceServiceImpl. Jul 16, 2022
@mum4k mum4k requested a review from eric846 July 16, 2022 21:58
@mum4k mum4k added the waiting-for-review A PR waiting for a review. label Jul 16, 2022
@mum4k
Copy link
Collaborator Author

mum4k commented Jul 16, 2022

@eric846 please review and assign back to me once done.

Copy link
Contributor

@eric846 eric846 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mum4k mum4k merged commit 339a96e into envoyproxy:main Jul 18, 2022
@mum4k mum4k deleted the test_gcc_fix branch July 18, 2022 13:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

waiting-for-review A PR waiting for a review.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

do_ci.sh test_gcc CI target fails

3 participants