Add setters for all OptionsImpl fields.#3424
Merged
Merged
Conversation
This allows modifying options in a custom main() before instantiating the server but leaving them readonly within the server's context. Signed-off-by: Trevor Schroeder <trevors@google.com>
Member
mattklein123
left a comment
mattklein123
left a comment
There was a problem hiding this comment.
LGTM, small super nit if you feel like it.
|
|
||
| // Server::Options | ||
| uint64_t baseId() const override { return base_id_; } | ||
| void setBaseId(uint64_t base_id) { base_id_ = base_id; }; |
Member
There was a problem hiding this comment.
super nit: since these aren't actually part of the interface, can you move the new methods above the Server::Options comment?
added 2 commits
May 17, 2018 16:54
Signed-off-by: Trevor Schroeder <trevors@google.com>
Signed-off-by: Trevor Schroeder <trevors@google.com>
mattklein123
approved these changes
May 18, 2018
junr03
added a commit
to envoyproxy/envoy-mobile
that referenced
this pull request
May 5, 2020
…nals (#835) Description: Disabling signal handling in the Server::Options makes it so that the server's event dispatcher does not listen for termination signals such as SIGTERM, SIGINT, etc. Previous crashes in iOS were experienced due to out-of-band event loop exit as described in #831. Ignoring termination signals makes it more likely that the event loop will only exit due to Engine destruction. This PR introduces Envoy::MobileMainCommon, as this is the canonical way to customize how main runs, and options setup per envoyproxy/envoy#3424. The new Envoy::MobileMainCommon also does away with other logic in Envoy::MainCommon that does not apply to Envoy Mobile. Risk Level: med - low-level change in termination handling Testing: unit test to assert option is correctly set. End to end test with iOS and Android devices to ensure clean exit when the app using envoy mobile exits (and thus destructs the engine). Moreover, there is no event loop exit any longer when the simulator app receives a SIGTERM, i.e., the event dispatcher is no longer listening to SIGTERM and exiting due to them. Potentially fixes #831. Will need to verify with wider client release. Signed-off-by: Jose Nino jnino@lyft.com
jpsim
pushed a commit
that referenced
this pull request
Nov 28, 2022
…nals (#835) Description: Disabling signal handling in the Server::Options makes it so that the server's event dispatcher does not listen for termination signals such as SIGTERM, SIGINT, etc. Previous crashes in iOS were experienced due to out-of-band event loop exit as described in #831. Ignoring termination signals makes it more likely that the event loop will only exit due to Engine destruction. This PR introduces Envoy::MobileMainCommon, as this is the canonical way to customize how main runs, and options setup per #3424. The new Envoy::MobileMainCommon also does away with other logic in Envoy::MainCommon that does not apply to Envoy Mobile. Risk Level: med - low-level change in termination handling Testing: unit test to assert option is correctly set. End to end test with iOS and Android devices to ensure clean exit when the app using envoy mobile exits (and thus destructs the engine). Moreover, there is no event loop exit any longer when the simulator app receives a SIGTERM, i.e., the event dispatcher is no longer listening to SIGTERM and exiting due to them. Potentially fixes #831. Will need to verify with wider client release. Signed-off-by: Jose Nino jnino@lyft.com Signed-off-by: JP Simard <jp@jpsim.com>
jpsim
pushed a commit
that referenced
this pull request
Nov 29, 2022
…nals (#835) Description: Disabling signal handling in the Server::Options makes it so that the server's event dispatcher does not listen for termination signals such as SIGTERM, SIGINT, etc. Previous crashes in iOS were experienced due to out-of-band event loop exit as described in #831. Ignoring termination signals makes it more likely that the event loop will only exit due to Engine destruction. This PR introduces Envoy::MobileMainCommon, as this is the canonical way to customize how main runs, and options setup per #3424. The new Envoy::MobileMainCommon also does away with other logic in Envoy::MainCommon that does not apply to Envoy Mobile. Risk Level: med - low-level change in termination handling Testing: unit test to assert option is correctly set. End to end test with iOS and Android devices to ensure clean exit when the app using envoy mobile exits (and thus destructs the engine). Moreover, there is no event loop exit any longer when the simulator app receives a SIGTERM, i.e., the event dispatcher is no longer listening to SIGTERM and exiting due to them. Potentially fixes #831. Will need to verify with wider client release. Signed-off-by: Jose Nino jnino@lyft.com Signed-off-by: JP Simard <jp@jpsim.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This allows modifying options in a custom main() before instantiating
the server but leaving them readonly within the server's context.
Signed-off-by: Trevor Schroeder trevors@google.com
main: allow options to be changed before starting the server
Description:
This allows custom main() functions to modify the Options before
starting the server instead of requiring setting everything on the
command line.
Risk Level: Low
Testing:
Additional tests were added for the feature and all tests under test/ were run.