Skip to content

listener: extract active_tcp_socket and active_stream_listener_base#17355

Merged
mattklein123 merged 29 commits intoenvoyproxy:mainfrom
lambdai:addinternallistener_pre_2
Jul 22, 2021
Merged

listener: extract active_tcp_socket and active_stream_listener_base#17355
mattklein123 merged 29 commits intoenvoyproxy:mainfrom
lambdai:addinternallistener_pre_2

Conversation

@lambdai
Copy link
Copy Markdown
Contributor

@lambdai lambdai commented Jul 15, 2021

Commit Message:

Split from #16947
Extract ActiveTcpSocket from ActiveTcpListener.
Extract a base class ActiveStreamListener from ActiveTcpListener.

Additional Description:
The new base class owns the the active sockets that drive itself through the listener filters.
After the active socket passes all the listener filters, a server connection is created. As a derived listener,
ActiveTcpListener overrides newActiveConnection to take the ownership of that server connection.

Risk Level: LOW
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Deprecated:]
[Optional API Considerations:]

lambdai and others added 23 commits June 1, 2021 10:57
Signed-off-by: Yuchen Dai <silentdai@gmail.com>
Signed-off-by: Yuchen Dai <silentdai@gmail.com>
Signed-off-by: Yuchen Dai <silentdai@gmail.com>
Signed-off-by: Yuchen Dai <silentdai@gmail.com>
Signed-off-by: Yuchen Dai <silentdai@gmail.com>
Signed-off-by: Yuchen Dai <silentdai@gmail.com>
Signed-off-by: Yuchen Dai <silentdai@gmail.com>
Signed-off-by: Yuchen Dai <silentdai@gmail.com>
Signed-off-by: Yuchen Dai <lambdai@google.com>
Signed-off-by: Yuchen Dai <lambdai@google.com>
Signed-off-by: Yuchen Dai <lambdai@google.com>
Signed-off-by: Yuchen Dai <lambdai@google.com>
Signed-off-by: Yuchen Dai <silentdai@gmail.com>
Signed-off-by: Yuchen Dai <silentdai@gmail.com>
and push up Network::Connection op

Signed-off-by: Yuchen Dai <silentdai@gmail.com>
Signed-off-by: Yuchen Dai <silentdai@gmail.com>
Signed-off-by: Yuchen Dai <silentdai@gmail.com>
Signed-off-by: Yuchen Dai <silentdai@gmail.com>
Signed-off-by: Yuchen Dai <silentdai@gmail.com>
…re_1

Signed-off-by: Yuchen Dai <silentdai@gmail.com>
Signed-off-by: Yuchen Dai <silentdai@gmail.com>
Signed-off-by: Yuchen Dai <silentdai@gmail.com>
@mattklein123 mattklein123 self-assigned this Jul 15, 2021
@mattklein123
Copy link
Copy Markdown
Member

Check CI?

/wait

Signed-off-by: Yuchen Dai <silentdai@gmail.com>
@lambdai
Copy link
Copy Markdown
Contributor Author

lambdai commented Jul 15, 2021

Ahrr, Add a setting to my vscode

Copy link
Copy Markdown
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Can you summarize what code you actually changed vs. moved? What do I need to review?

/wait-any

@lambdai
Copy link
Copy Markdown
Contributor Author

lambdai commented Jul 16, 2021

Can you summarize what code you actually changed vs. moved? What do I need to review?

/wait-any

Sure.

  1. active_tcp_socket.{h,cc} It's a pure move other than referring a parent class of ActiveTcpListener.
  2. ActiveStreamListenerBase is the new extracted parent class of ActiveTcpListener. Sitting in it's new file active_stream_listener_base.{h, cc}. Its interface a strict subset of the previous ActiveTcpListener. The only exception is newActiveConnection that is designed to be overridden by ActiveTcpListener.
    Particularly, no data member is added to ActiveTcpListener+ActiveStreamListenerBase. All the functions have the exact semantic before the refactor.

The goal is to avoid duplicating the ActiveTcpListener when a ActiveInternalListener is added.

Without the context of the above goal, this PR should be small win regarding the achievement of less coupling between ActiveTcpSocket and ActiveTcpListener.

Copy link
Copy Markdown
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Thanks, generally makes sense with some random comments.

/wait

lambdai added 2 commits July 22, 2021 00:13
Signed-off-by: Yuchen Dai <silentdai@gmail.com>
Signed-off-by: Yuchen Dai <silentdai@gmail.com>
@lambdai
Copy link
Copy Markdown
Contributor Author

lambdai commented Jul 22, 2021

fixing conflict
/wait

Signed-off-by: Yuchen Dai <silentdai@gmail.com>
Signed-off-by: Yuchen Dai <silentdai@gmail.com>
@mattklein123
Copy link
Copy Markdown
Member

OK LGTM pending the other open comment thread about moving to protected, thanks.

/wait

@lambdai
Copy link
Copy Markdown
Contributor Author

lambdai commented Jul 22, 2021

OK LGTM pending the other open comment thread about moving to protected, thanks.

/wait

I had the illusion it is done.

Now I am trying to move to protected and I realize it was public even before this PR.

This things I am doing is:

  1. declare it protected.
  2. add const getter. it is for test only, I can take a look how to avoid using it in the test.
  3. add a remove method in this class to mutate this list.

Signed-off-by: Yuchen Dai <silentdai@gmail.com>
Copy link
Copy Markdown
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Thanks!

@mattklein123 mattklein123 merged commit 6738fea into envoyproxy:main Jul 22, 2021
leyao-daily pushed a commit to leyao-daily/envoy that referenced this pull request Sep 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants