Begin process of removing singleton use by StringMatcher#32829
Begin process of removing singleton use by StringMatcher#32829ggreenway merged 15 commits intoenvoyproxy:mainfrom
Conversation
Signed-off-by: Greg Greenway <ggreenway@apple.com>
doesn't use singletons. Add Regex to the factory context interface. Signed-off-by: Greg Greenway <ggreenway@apple.com>
Signed-off-by: Greg Greenway <ggreenway@apple.com>
Signed-off-by: Greg Greenway <ggreenway@apple.com>
Signed-off-by: Greg Greenway <ggreenway@apple.com>
Signed-off-by: Greg Greenway <ggreenway@apple.com>
Signed-off-by: Greg Greenway <ggreenway@apple.com>
Signed-off-by: Greg Greenway <ggreenway@apple.com>
Signed-off-by: Greg Greenway <ggreenway@apple.com>
Signed-off-by: Greg Greenway <ggreenway@apple.com>
|
/retest |
RyanTheOptimist
left a comment
There was a problem hiding this comment.
Looks reasonable, but I think ASAN is finding some correctness issues, eg:
source/common/common/matchers.h:104:66: runtime error: reference binding to null pointer of type 'Regex::Engine'
Signed-off-by: Greg Greenway <ggreenway@apple.com>
Signed-off-by: Greg Greenway <ggreenway@apple.com>
Yep, some instances where I converted a nullptr to a reference. The reference was never used, but still gets flagged by asan. I think I've worked around it now, but waiting on CI. There was also a real failure on mobile that I think I've fixed. |
Signed-off-by: Greg Greenway <ggreenway@apple.com>
`regexEngine()` isn't yet called in the config validation path that tests hit Signed-off-by: Greg Greenway <ggreenway@apple.com>
|
CC @envoyproxy/coverage-shephards: FYI only for changes made to |
RyanTheOptimist
left a comment
There was a problem hiding this comment.
LGTM, modulo coverage. But maybe that's hard to fix?
| "source/extensions/listener_managers/validation_listener_manager:70.5" | ||
| "source/extensions/watchdog/profile_action:83.3" | ||
| "source/server:91.0" # flaky: be careful adjusting. See https://github.com/envoyproxy/envoy/issues/15239 | ||
| "source/server/config_validation:89.2" |
There was a problem hiding this comment.
Any idea why this changed/how hard it would be to improve?
There was a problem hiding this comment.
I added regexEngine() to the Server::Instance. This isn't used (yet) in the config_validation path, because most of the uses of StringMatcherImpl still don't use it. Future PRs will convert all of those, and this line will get hit in coverage at that time.
|
/retest |
…oyproxy#32829)" This reverts commit b7818b0. Signed-off-by: Ryan Northey <ryan@synca.io>
Commit Message: Begin process of removing singleton use by StringMatcher
This PR temporarily adds two versions of StringMatcherImpl: one that uses the existing singletons, and one that takes a factory context.
Subsequent PRs will gradually convert all uses of StringMatcherImpl to use the factory context. Then the version using singletons will be deleted, along with the code that sets the singletons.
Additional Description: This work is being broken into multiple PRs because otherwise it would be very large and difficult to review.
Part of #32792
Risk Level: low
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]