Skip to content

Update Envoy to dee7021 (Aug 18th 2021).#736

Merged
oschaaf merged 3 commits intomainfrom
mum4k_envoy_update_010_async_client_race
Aug 19, 2021
Merged

Update Envoy to dee7021 (Aug 18th 2021).#736
oschaaf merged 3 commits intomainfrom
mum4k_envoy_update_010_async_client_race

Conversation

@mum4k
Copy link
Collaborator

@mum4k mum4k commented Aug 19, 2021

  • Envoy now demands that Envoy::Grpc::AsyncClient is created, used and destroyed on the same thread. (See
    [grpc]: fix ex_authz grpc client race condition envoy#17619). Nighthawk creates the Envoy::Grpc::AsyncClient on the worker thread, but destroys it on the main which triggers an assertion failure. Fixing this by:
    • Adding a new method RequestSource::destroyOnThread() which complements the pre-existing RequestSource::initOnThread(). While the latter gets called when a worker thread starts, the former now gets called when a worker thread is shutting down.
    • Implementing RequestSource::destroyOnThread() on the only request source implementation that uses Envoy::Grpc::AsyncClient, i.e. Nighthawk::RemoteRequestSourceImpl, the method destroys the object that
      owns Envoy::Grpc::AsyncClient when called.
  • no changes to .bazelrc, .bazelversion, run_envoy_docker.sh.
  • lowering the coverage threshold to 93.2 to accommodate the fact that this code path is only covered in integration tests. The unit-tests use a mock.

mum4k added 2 commits August 19, 2021 01:06
Signed-off-by: Jakub Sobon <mumak@google.com>
Signed-off-by: Jakub Sobon <mumak@google.com>
@mum4k mum4k requested a review from dubious90 August 19, 2021 06:33
@mum4k mum4k added the waiting-for-review A PR waiting for a review. label Aug 19, 2021
oschaaf
oschaaf previously approved these changes Aug 19, 2021
Copy link
Member

@oschaaf oschaaf left a comment

Choose a reason for hiding this comment

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

the coverage test seems to indicate we started trending low, I'd be surprised if this PR pushed it over the edge, could that be a flake somehow?
That aside, this LGTM

Signed-off-by: Jakub Sobon <mumak@google.com>
@mum4k
Copy link
Collaborator Author

mum4k commented Aug 19, 2021

Adjusted the coverage threshold and included explanation in the PR description.

PTAL

@oschaaf oschaaf merged commit 3ea0392 into main Aug 19, 2021
@mum4k mum4k deleted the mum4k_envoy_update_010_async_client_race branch August 19, 2021 08:07
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.

2 participants