Add a ThreadFactory class for creating threads #5055
Merged
Conversation
This file contains 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
This enables injecting a custom thread factory into the envoy server Signed-off-by: Elisha Ziskind <eziskind@google.com>
|
@jmarantz PTAL since you had concerns with our previous approach. |
jmarantz
reviewed
Nov 18, 2018
|
mac failure is a flake: |
|
Sigh, more flakes, under ipv6 this time: |
jmarantz
reviewed
Nov 19, 2018
jmarantz
previously approved these changes
Nov 19, 2018
Signed-off-by: Elisha Ziskind <eziskind@google.com>
mattklein123
previously approved these changes
Nov 20, 2018
Signed-off-by: Elisha Ziskind <eziskind@google.com>
eziskind
changed the title
htuch
pushed a commit
that referenced
this pull request
Nov 21, 2018
For the record, I am against singletons in general, and believe it's more maintainable to pass context objects through the call/instantiation stack, as is done in Envoy production code. However the test system employs a deep system of mocks that are constructed without context. As a result, any new context that may be needed in the future (e.g. SymbolTable in #4980, ThreadSystem in #5055 and #5072 and FileSystem in #5031) need a mechanism to plumb data into the system as instantiated during tests. I believe Api::OsSysCalls() is another example that might benefit from this. This class provides a mechanism to have a bounded-scope singleton, such that anywhere an instance of a type T is desired, the same instance will be available. Once all references to a Global go out of scope, the shared instance is destroyed and the pointer to it nulled. Thus, this singleton pattern has the property that between tests, the state will be completely reset, as long as there are no memory leaks. The new class, Test::Global, creates a potential conflict with tests that had declared using testing::Test;, so this PR also cleans those up and adds a format check to avoid having that pattern re-occur. Risk Level: the main risk here is that this class will be abused and used in lieu of proper object passing when that's most appropriate. However, the new library is declared as a bazel test library and is in the test/... tree, so hopefully -- with proper code reviews, the usage will be limited. Testing: //test/... Signed-off-by: Joshua Marantz <jmarantz@google.com>
fredlas
pushed a commit
to fredlas/envoy
that referenced
this pull request
Mar 5, 2019
This enables injecting a custom thread factory into the envoy server Signed-off-by: Elisha Ziskind <eziskind@google.com> Signed-off-by: Fred Douglas <fredlas@google.com>
fredlas
pushed a commit
to fredlas/envoy
that referenced
this pull request
Mar 5, 2019
For the record, I am against singletons in general, and believe it's more maintainable to pass context objects through the call/instantiation stack, as is done in Envoy production code. However the test system employs a deep system of mocks that are constructed without context. As a result, any new context that may be needed in the future (e.g. SymbolTable in envoyproxy#4980, ThreadSystem in envoyproxy#5055 and envoyproxy#5072 and FileSystem in envoyproxy#5031) need a mechanism to plumb data into the system as instantiated during tests. I believe Api::OsSysCalls() is another example that might benefit from this. This class provides a mechanism to have a bounded-scope singleton, such that anywhere an instance of a type T is desired, the same instance will be available. Once all references to a Global go out of scope, the shared instance is destroyed and the pointer to it nulled. Thus, this singleton pattern has the property that between tests, the state will be completely reset, as long as there are no memory leaks. The new class, Test::Global, creates a potential conflict with tests that had declared using testing::Test;, so this PR also cleans those up and adds a format check to avoid having that pattern re-occur. Risk Level: the main risk here is that this class will be abused and used in lieu of proper object passing when that's most appropriate. However, the new library is declared as a bazel test library and is in the test/... tree, so hopefully -- with proper code reviews, the usage will be limited. Testing: //test/... Signed-off-by: Joshua Marantz <jmarantz@google.com> Signed-off-by: Fred Douglas <fredlas@google.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description: This change modifies ApiImpl to delegate thread creation to a ThreadFactory instance and enables injecting a custom thread factory into the envoy server by wiring one in through MainCommonBase
Risk Level: low (mostly code refactoring)
Testing: unit and integration tests
Signed-off-by: Elisha Ziskind eziskind@google.com