listener: move active connection collection out of active tcp listener by lambdai · Pull Request #16947 · envoyproxy/envoy

lambdai · 2021-06-11T23:02:50Z

Commit Message:

#16763 is adding an internal listener. The internal listener is not driven by event dispatcher.
However, it shares the common functionalities of structuring the ownership of connections with the tcp listener.

This PR extracts the common functions out of ActiveTcpListener so those can be reused by internal listener.
The common includes

ActiveConnection definition
The helper method of remove an active connection and remove an filter chain

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

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

Signed-off-by: Yuchen Dai <lambdai@google.com>

antoniovicente

Thanks for splitting this refactor from #16763. This change is significantly more possible to review.

source/server/active_stream_socket.h

source/server/active_tcp_listener.h

source/server/active_stream_socket.h

source/server/active_tcp_listener.cc

source/server/active_stream_socket.h

Signed-off-by: Yuchen Dai <lambdai@google.com>

lambdai

Thank you for the review! I addressed most of them except the inc/decConnections.

source/server/active_stream_socket.h

source/server/active_tcp_listener.cc

source/server/active_tcp_listener.h

source/server/active_stream_socket.h

lambdai · 2021-06-15T20:55:27Z

/wait

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

source/server/active_tcp_socket.h

source/server/active_tcp_listener.cc

source/server/BUILD

source/server/active_stream_socket.h

source/server/active_tcp_listener.h

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

and push up Network::Connection op Signed-off-by: Yuchen Dai <silentdai@gmail.com>

source/server/active_tcp_socket.h

source/server/active_tcp_listener.h

antoniovicente

Yeah I understand it's just hard to track what is changing. My preference would be to wait until next week to see if @antoniovicente can finish this review. If that is not possible, would it be possible to just split again where you do the pure moves first (I think ActiveTcpSocket into its own file, etc.) and then the rest of it?

I'ld like a second opinion about the introduction of a template base class for code reuse. This kind of pattern has been a big source of complexity on past codebases I have been involved with. @mattklein123 could use your opinion. Other than that, I think this change is fine and is about as minimal as it can be except possibly the refactor to source/server/active_tcp_listener.h which exposes the inner classes.

source/server/active_stream_listener_base.h

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

lambdai · 2021-07-28T19:56:26Z

This PR is ready to be reviewed again: It turns to be a create a thin reused layer OwnedActiveStreamListenerBase

With this base class, ActiveConnection can be reused by both internal and tcp listener. And the error prone shared connection handling block are moved to here.

mattklein123

Please split out pure moves from code changes so it's easier to review the actual changes. Thank you!

/wait

lambdai · 2021-07-30T18:00:03Z

@mattklein123 The entire PR is almost code move(file to file), or method move(from class to base class) or rename(refer to base class instead of derived).

I create a PR as your requested anyway: #17551

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

source/server/active_stream_listener_base.h

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

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

mattklein123

Thanks!

…bridge-stream * upstream/main: (32 commits) tls: move ssl connection info into SocketAddressProvider (envoyproxy#17334) conn pool: default enable runtime feature `conn_pool_delete_when_idle` (envoyproxy#17577) api: LEDS api introduction (envoyproxy#17419) kafka: add support for api versions request in mesh-filter (envoyproxy#17475) ext_proc: Implement BUFFERED_PARTIAL processing mode (envoyproxy#17531) tooling: Async/pathlib/mypy cleanups and utils (envoyproxy#17505) xds: restructure CertificateProvider fields (envoyproxy#17201) Refactor OverloadIntegrationTest breaking out a test base, and the fake resource monitors. (envoyproxy#17530) listener: move active connection collection out of active tcp listener (envoyproxy#16947) tools: format checks for backticks (envoyproxy#17566) coverage: set lower limit for common/quic and common (envoyproxy#17573) v2: final source removal (envoyproxy#17565) test: bumping coverage (envoyproxy#17564) quic: enforcing header size and contents (envoyproxy#17520) Support for canonicalizing URI properly for AWS SigV4 signer (envoyproxy#17137) listener: add a stat for transport socket connect timeout (envoyproxy#17458) listener: add listen() error handling (envoyproxy#17427) http: return per route config when direct response is set (envoyproxy#17449) removing most v2 references from source/ (envoyproxy#17415) bug fix: return bootstrap when validating config (envoyproxy#17499) ... Signed-off-by: Garrett Bourg <bourg@squareup.com>

envoyproxy#16947) Signed-off-by: Yuchen Dai <silentdai@gmail.com>

lambdai and others added 9 commits June 1, 2021 10:57

initial internal listener

cb82711

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

for new pr

4c156ce

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

merge main

773c75a

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

delete test/integration/internal_tcp_proxy_integration_test.cc

b6ff892

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

coverage

635f185

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

remove ClientConnectionImpl ctor for internal socket

3615512

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

revert dispatcher

efe9ce7

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

merged main

01ee5bb

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

revert internal listener

1564151

Signed-off-by: Yuchen Dai <lambdai@google.com>

lambdai assigned antoniovicente Jun 11, 2021

lambdai added 2 commits June 11, 2021 23:04

pragma once

9e5b2b4

Signed-off-by: Yuchen Dai <lambdai@google.com>

test bootstrap

e802a6f

Signed-off-by: Yuchen Dai <lambdai@google.com>

lambdai mentioned this pull request Jun 12, 2021

Listener: implement internal listener #16763

Closed

antoniovicente reviewed Jun 12, 2021

View reviewed changes

antoniovicente added the waiting label Jun 14, 2021

comment other than incNumConnections

c8e4e43

Signed-off-by: Yuchen Dai <lambdai@google.com>

repokitteh-read-only bot removed the waiting label Jun 15, 2021

lambdai commented Jun 15, 2021

View reviewed changes

repokitteh-read-only bot added the waiting label Jun 15, 2021

rename

aac23c1

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

repokitteh-read-only bot removed the waiting label Jun 15, 2021

antoniovicente reviewed Jun 16, 2021

View reviewed changes

source/server/active_tcp_socket.h Outdated Show resolved Hide resolved

source/server/active_tcp_listener.cc Outdated Show resolved Hide resolved

antoniovicente reviewed Jun 16, 2021

View reviewed changes

source/server/BUILD Outdated Show resolved Hide resolved

source/server/active_stream_socket.h Outdated Show resolved Hide resolved

source/server/active_tcp_listener.h Outdated Show resolved Hide resolved

antoniovicente added the waiting label Jun 16, 2021

soulxu reviewed Jun 16, 2021

View reviewed changes

source/server/active_tcp_listener.h Outdated Show resolved Hide resolved

lambdai added 2 commits June 17, 2021 07:59

put cleanup, deferdelete in base

573cdfe

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

push down conn collection or active conn op

c1f7921

and push up Network::Connection op Signed-off-by: Yuchen Dai <silentdai@gmail.com>

repokitteh-read-only bot removed the waiting label Jun 17, 2021

antoniovicente reviewed Jun 18, 2021

View reviewed changes

source/server/active_tcp_socket.h Outdated Show resolved Hide resolved

source/server/active_tcp_socket.h Outdated Show resolved Hide resolved

source/server/active_tcp_listener.h Outdated Show resolved Hide resolved

antoniovicente reviewed Jul 27, 2021

View reviewed changes

source/server/active_stream_listener_base.h Outdated Show resolved Hide resolved

add comment

aeb163a

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

repokitteh-read-only bot added the waiting label Jul 27, 2021

comment to TypedActiveStreamListenerBase

056f51a

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

repokitteh-read-only bot added waiting and removed waiting labels Jul 27, 2021

final

f0fde02

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

repokitteh-read-only bot removed the waiting label Jul 28, 2021

lambdai changed the title ~~listener: refactor active tcp socket and active tcp listener~~ listener: move active connection collection out of active tcp listener Jul 28, 2021

another method move

bfdf05a

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

mattklein123 requested changes Jul 30, 2021

View reviewed changes

repokitteh-read-only bot added the waiting label Jul 30, 2021

lambdai mentioned this pull request Jul 30, 2021

listener: active connection move only #17551

Merged

lambdai removed the waiting label Jul 30, 2021

mattklein123 added the waiting label Jul 30, 2021

Merge branch 'main' into addinternallistener_pre_1

02e618c

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

repokitteh-read-only bot removed the waiting label Jul 30, 2021

mattklein123 reviewed Aug 2, 2021

View reviewed changes

source/server/active_stream_listener_base.h Show resolved Hide resolved

repokitteh-read-only bot added waiting:any waiting and removed waiting:any labels Aug 2, 2021

lambdai added 2 commits August 3, 2021 18:20

Merge branch 'main' into addinternallistener_pre_1

bd122fa

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

Merge remote-tracking branch 'me/addinternallistener_pre_1' into addi…

2f406f5

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

repokitteh-read-only bot removed the waiting label Aug 3, 2021

mattklein123 approved these changes Aug 3, 2021

View reviewed changes

mattklein123 merged commit 75b7c07 into envoyproxy:main Aug 3, 2021

leyao-daily pushed a commit to leyao-daily/envoy that referenced this pull request Sep 30, 2021

listener: move active connection collection out of active tcp listener (

02d1665

envoyproxy#16947) Signed-off-by: Yuchen Dai <silentdai@gmail.com>

Conversation

lambdai commented Jun 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

antoniovicente left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lambdai left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lambdai commented Jun 15, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

antoniovicente left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lambdai commented Jul 28, 2021

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

lambdai commented Jul 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

lambdai commented Jun 11, 2021 •

edited

Loading

lambdai commented Jul 30, 2021 •

edited

Loading