refactor: active listener and connection handler #15127
Conversation
Signed-off-by: Yuchen Dai <[email protected]>
Signed-off-by: Yuchen Dai <[email protected]> add TcpConnectionHandlerHelper Signed-off-by: Yuchen Dai <[email protected]> fix udp listeners Signed-off-by: Yuchen Dai <[email protected]> remove unused MockConnectionHandler Signed-off-by: Yuchen Dai <[email protected]> TcpConnectionHandler and UdpConnectionHandler Signed-off-by: Yuchen Dai <[email protected]> lessen requirement of activeTcpListener Signed-off-by: Yuchen Dai <[email protected]> add TcpConnectionHandler and ActiveTcpListener doesn't rely on impl of ConnHandler Signed-off-by: Yuchen Dai <[email protected]> remove more mock conn handler Signed-off-by: Yuchen Dai <[email protected]>
Signed-off-by: Yuchen Dai <[email protected]>
lambdai
changed the title
Signed-off-by: Yuchen Dai <[email protected]>
lambdai
requested review from
alyssawilk,
asraa,
ggreenway,
lizan,
mattklein123 and
PiotrSikora
as code owners
| * @param hand_off_restored_destination_connections true if redirection is allowed. | ||
| * @param rebalanced true if no more balance is allowed. | ||
| */ | ||
| virtual void onAcceptWorker(Network::ConnectionSocketPtr&& socket, |
There was a problem hiding this comment.
Extracted from ConnectionHandlerImpl
| /** | ||
| * The connection handler from the view of a tcp listener. | ||
| */ | ||
| class TcpConnectionHandler : public virtual ConnectionHandler { |
There was a problem hiding this comment.
Extracted from ConnectionHandlerImpl
| /** | ||
| * Wrapper for an active listener owned by this handler. | ||
| */ | ||
| class ActiveListenerImplBase : public virtual Network::ConnectionHandler::ActiveListener { |
There was a problem hiding this comment.
Moved out of ConnectionHandlerImpl
| class ActiveRawUdpListener : public ActiveUdpListenerBase, | ||
| public Network::UdpListenerFilterManager, | ||
| public Network::UdpReadFilterCallbacks { | ||
| class ActiveListenerImplBase : public virtual Network::ConnectionHandler::ActiveListener { |
There was a problem hiding this comment.
Move out of ConnectionHandlerImpl because ActiveListenerImplBase is used by both Active{Tcp,Udp}Listener. And the latter two classes are moved to separate file
|
@ggreenway as the current reviewer I didn't touch connection_handler_test because I want to prove the refactor is logically correct. Right now the PR is now "+1,096 −968" :( |
There was a problem hiding this comment.
This is difficult to review mostly due to code moving to different files. Can you break this into two PRs (or at least two commits in this PR), one that moves code into the correct classes, and one that moves those changes between files only (only cut/paste + bazel changes)?
|
@ggreenway I feel your pain. I will start with extracting the internal classes. That are risk-free but account for the majority of the diff |
|
/wait |
|
This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
github-actions
bot
added
the
stale
|
Close this one: Landed in split PRs |
Commit Message:
Refactor ConnectionHandler: separate inferfaces for tcp listener and udp listener.
Additional Description:
Since an active listener doesn't depend on ConnectionImpl, the complexity of writing a functional test of an active tcp listener is greatly reduced.
Also easier to add a new type of listener wrt my ongoing work to add the envoy internal address listener which has a quite different "accept()" flow and findListenerByAddress implementation.
Risk Level:
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
Fixes #15126
[Optional Deprecated:]
[Optional API Considerations:]