Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

api listener: add shutdown method and call during server termination #9959

Merged
merged 8 commits into from
Feb 8, 2020
Merged

api listener: add shutdown method and call during server termination #9959

merged 8 commits into from
Feb 8, 2020

Conversation

junr03
Copy link
Member

@junr03 junr03 commented Feb 7, 2020

Description: this PR adds a shutdown method to the ApiListener and calls it where appropriate during server termination. Previously there would be a crash due to use after free of objects in thread local storage by streams in the ApiListener. Funny enough the flakes reported in #9746 happened due to this.
Risk Level: low
Testing: new unit and integration test. Without appropriate termination the new integration test repros the stacktrace reported in #9746.

Signed-off-by: Jose Nino jnino@lyft.com

Jose Nino added 5 commits February 6, 2020 16:15
Signed-off-by: Jose Nino <jnino@lyft.com>
Signed-off-by: Jose Nino <jnino@lyft.com>
fmt
Signed-off-by: Jose Nino <jnino@lyft.com>
Signed-off-by: Jose Nino <jnino@lyft.com>
Signed-off-by: Jose Nino <jnino@lyft.com>
Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

LGTM with small comments, thank you!

/wait

include/envoy/server/api_listener.h Outdated Show resolved Hide resolved
source/server/api_listener_impl.cc Outdated Show resolved Hide resolved
test/server/api_listener_test.cc Outdated Show resolved Hide resolved
Signed-off-by: Jose Nino <jnino@lyft.com>
@junr03
Copy link
Member Author

junr03 commented Feb 7, 2020

@mattklein123 updated. Thanks!

fix
Signed-off-by: Jose Nino <jnino@lyft.com>
Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

LGTM other than small typo, thanks!

/wait

include/envoy/server/api_listener.h Outdated Show resolved Hide resolved
Signed-off-by: Jose Nino <jnino@lyft.com>
Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Thanks!

@junr03 junr03 merged commit 9105aea into envoyproxy:master Feb 8, 2020
rebello95 added a commit to envoyproxy/envoy-mobile that referenced this pull request Feb 10, 2020
Bumping to include the following fixes:
- `dns: destroy/reinitialize c-ares channel on ARES_ECONNREFUSED`: envoyproxy/envoy#9899. This should resolve the issues we've been seeing with Envoy Mobile hanging on startup and never properly issuing requests if started in the offline state on iOS. Fixes #672, though more improvements to DNS resolution will be investigated in #673
- `gzip: add force load factory declaration`: envoyproxy/envoy#9942. This will allow us to use the gzip filter with Envoy Mobile
- `api listener: add shutdown method and call during server termination`: envoyproxy/envoy#9959. Fixes #667. Fixes #674

Includes the following PRs to fix iOS liveliness tests as well:
- `metrics service: force link v2 config`: envoyproxy/envoy#9875
- `config: remove ApiTypeOracle assert`: envoyproxy/envoy#9973

Also contains necessary updates to accommodate [this change](envoyproxy/envoy@dea4eb0).

Signed-off-by: Michael Rebello <me@michaelrebello.com>
jpsim pushed a commit that referenced this pull request Nov 28, 2022
Bumping to include the following fixes:
- `dns: destroy/reinitialize c-ares channel on ARES_ECONNREFUSED`: #9899. This should resolve the issues we've been seeing with Envoy Mobile hanging on startup and never properly issuing requests if started in the offline state on iOS. Fixes envoyproxy/envoy-mobile#672, though more improvements to DNS resolution will be investigated in envoyproxy/envoy-mobile#673
- `gzip: add force load factory declaration`: #9942. This will allow us to use the gzip filter with Envoy Mobile
- `api listener: add shutdown method and call during server termination`: #9959. Fixes envoyproxy/envoy-mobile#667. Fixes envoyproxy/envoy-mobile#674

Includes the following PRs to fix iOS liveliness tests as well:
- `metrics service: force link v2 config`: #9875
- `config: remove ApiTypeOracle assert`: #9973

Also contains necessary updates to accommodate [this change](dea4eb0).

Signed-off-by: Michael Rebello <me@michaelrebello.com>
Signed-off-by: JP Simard <jp@jpsim.com>
jpsim pushed a commit that referenced this pull request Nov 29, 2022
Bumping to include the following fixes:
- `dns: destroy/reinitialize c-ares channel on ARES_ECONNREFUSED`: #9899. This should resolve the issues we've been seeing with Envoy Mobile hanging on startup and never properly issuing requests if started in the offline state on iOS. Fixes envoyproxy/envoy-mobile#672, though more improvements to DNS resolution will be investigated in envoyproxy/envoy-mobile#673
- `gzip: add force load factory declaration`: #9942. This will allow us to use the gzip filter with Envoy Mobile
- `api listener: add shutdown method and call during server termination`: #9959. Fixes envoyproxy/envoy-mobile#667. Fixes envoyproxy/envoy-mobile#674

Includes the following PRs to fix iOS liveliness tests as well:
- `metrics service: force link v2 config`: #9875
- `config: remove ApiTypeOracle assert`: #9973

Also contains necessary updates to accommodate [this change](dea4eb0).

Signed-off-by: Michael Rebello <me@michaelrebello.com>
Signed-off-by: JP Simard <jp@jpsim.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants