Skip to content

Revert "Using map to store the listener in ConnectionHandlerImpl"#19811

Closed
soulxu wants to merge 1 commit intoenvoyproxy:mainfrom
soulxu:revert-19362-using_map_for_conn_handler
Closed

Revert "Using map to store the listener in ConnectionHandlerImpl"#19811
soulxu wants to merge 1 commit intoenvoyproxy:mainfrom
soulxu:revert-19362-using_map_for_conn_handler

Conversation

@soulxu
Copy link
Copy Markdown
Member

@soulxu soulxu commented Feb 4, 2022

Reverts #19362

Since it is failling istio testsuits istio/istio#37004, getting the listener mix up. Before figure out what is happening here, let us revert the change.

@soulxu soulxu requested a review from alyssawilk as a code owner February 4, 2022 00:23
@soulxu
Copy link
Copy Markdown
Member Author

soulxu commented Feb 4, 2022

@howardjohn let's revert it, looks like need some time to figure out what is happening here. thanks for helping on the test.

lambdai
lambdai previously approved these changes Feb 4, 2022
Copy link
Copy Markdown
Contributor

@lambdai lambdai left a comment

Choose a reason for hiding this comment

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

Thank you! I will triage after

Sorry for holding your further PRs on top of this one

@ggreenway
Copy link
Copy Markdown
Member

@soulxu can you add DCO signed-off-by to the commit please? You'll have to force-push to make DCO-bot happy.

…oyproxy#19362)"

This reverts commit 67c2b72.

Signed-off-by: He Jie Xu <hejie.xu@intel.com>
@soulxu soulxu force-pushed the revert-19362-using_map_for_conn_handler branch from 1de262f to 2d535ed Compare February 4, 2022 04:22
@soulxu
Copy link
Copy Markdown
Member Author

soulxu commented Feb 4, 2022

@soulxu can you add DCO signed-off-by to the commit please? You'll have to force-push to make DCO-bot happy.

yea, just done that

@soulxu
Copy link
Copy Markdown
Member Author

soulxu commented Feb 4, 2022

Thank you! I will triage after

Sorry for holding your further PRs on top of this one

thanks for helping me here

@soulxu
Copy link
Copy Markdown
Member Author

soulxu commented Feb 7, 2022

/wait

I found out the root cause, working on a fix.

}

Address::InstanceConstSharedPtr Utility::getIpv4AnyAddress(uint32_t port) {
CONSTRUCT_ON_FIRST_USE(Address::InstanceConstSharedPtr, new Address::Ipv4Instance(port));
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I mess up with this. CONSTRUCT_ON_FIRST_USE will create static object for Ipv4Instance, so it is going to build the first time with the parameter passed in first time also.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I found the same.
My local patch is to always return a newly created shared ptr. That will utilize the map lookup rather than iterating the map

https://github.com/lambdai/envoy-dai/blob/debugbreak0204/source/common/network/utility.cc#L349-L355

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

yea, that is.

I come up a fix #19802, but free to submit one if you want to since you already spend time on it.

Thanks for helping on this again!

@soulxu
Copy link
Copy Markdown
Member Author

soulxu commented Feb 7, 2022

Fix is here #19802, but free to merge this revert PR if this is better option.

@soulxu
Copy link
Copy Markdown
Member Author

soulxu commented Feb 9, 2022

Close this PR, since we have a fix.

@soulxu soulxu closed this Feb 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants