http filters: pass configs by reference instead of shared_ptr#14737
Closed
rgs1 wants to merge 10 commits intoenvoyproxy:mainfrom
Closed
http filters: pass configs by reference instead of shared_ptr#14737rgs1 wants to merge 10 commits intoenvoyproxy:mainfrom
rgs1 wants to merge 10 commits intoenvoyproxy:mainfrom
Conversation
Similar as envoyproxy#14711, but for HTTP filters. This change has two goals: 1) consistently deal with filter configs across all HTTP filters, thus making it clear that it's _fine_ to refence objects bound to the FilterFactoryCb 2) avoid confusing future filter writers who might think they need shared_ptrs when they really don't. I get confused every 6 months, so this includes me As a very small bonus, we create fewer shared_ptrs on every request though this won't show up in a profiler. It still makes me happy :-) Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>
rgs1
requested review from
alyssawilk,
dio,
htuch,
lizan,
mattklein123,
snowp and
zuercher
as code owners
added 5 commits
January 18, 2021 14:24
Missed part of this in the 1st pass. Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>
Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>
Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>
I _think_ some integration tests are not properly keeping the FilterFactoryCb around (as they should), which is causing issues. We can clean those up in a follow-up. Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>
added 4 commits
January 19, 2021 07:57
Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>
Turns out that keeping a reference to an object stored in the lambda itself causes issues, but if it's a shared_ptr it's fine. I am guessing that the FilterFactoryCb gets moved around so the reference goes stale. Thus we need to bind shared_ptrs to it. Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>
Member
|
Based on discussion in https://envoyproxy.slack.com/archives/C78HA81DH/p1611093479022600 I think we are going in a different direction here so I'm going to close this and @rgs1 is going to work on a different proposal! |
|
Is it still true that HTTP filters can reference objects bound to the FilterFactoryCb and the FilterFactoryCb lambda always outlives the filters it creates? It seems that there are no new proposals coming out yet... |
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
4 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.
Similar as #14711, but for HTTP filters.
This change has two goals:
thus making it clear that it's fine to refence objects bound
to the FilterFactoryCb
shared_ptrs when they really don't. I get confused every 6 months,
so this includes me
As a very small bonus, we create fewer shared_ptrs on every request
though this won't show up in a profiler. It still makes me happy :-)
Signed-off-by: Raul Gutierrez Segales rgs@pinterest.com