Conversation
zhiyipanuber
force-pushed
the
zhiyi.pan/mptcp
branch
from
c20b5c0 to
6b6241f
Compare
channel.go
Outdated
| var l net.Listener | ||
| var err error | ||
| if ch.enableMPTCP { | ||
| lc := &net.ListenConfig{} | ||
| lc.SetMultipathTCP(true) | ||
| l, err = lc.Listen(context.Background(), "tcp", hostPort) | ||
| } else { | ||
| l, err = net.Listen("tcp", hostPort) | ||
| } |
There was a problem hiding this comment.
First, I can confirm the correctness of existing code. The source code:
func Listen(network, address string) (Listener, error) {
var lc ListenConfig
return lc.Listen(context.Background(), network, address)
}
We can simplify this block and skip the if/else check. When the option is false, it is safe to set it as well.
lc := &net.ListenConfig{}
lc.SetMultipathTCP(ch.enableMPTCP)
l, err := lc.Listen(context.Background(), "tcp", hostPort)
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## dev #898 +/- ##
==========================================
+ Coverage 88.78% 89.14% +0.36%
==========================================
Files 43 43
Lines 4440 4514 +74
==========================================
+ Hits 3942 4024 +82
+ Misses 379 374 -5
+ Partials 119 116 -3
☔ View full report in Codecov by Sentry. |
zhiyipanuber
force-pushed
the
zhiyi.pan/mptcp
branch
from
aab00ca to
66fb29f
Compare
zhiyipanuber
force-pushed
the
zhiyi.pan/mptcp
branch
from
66fb29f to
810b98d
Compare
AllenLuUber
left a comment
AllenLuUber
left a comment
There was a problem hiding this comment.
Please get approval from another reviewer
| if opts.Dialer != nil { | ||
| dialCtx = func(ctx context.Context, hostPort string) (net.Conn, error) { | ||
| return opts.Dialer(ctx, "tcp", hostPort) |
There was a problem hiding this comment.
what happens if I set both EnableMPTCP and Dialer?
channel.go
Outdated
| } | ||
|
|
||
| l, err := net.Listen("tcp", hostPort) | ||
| lc := &net.ListenConfig{} |
There was a problem hiding this comment.
@AllenLuUber @jronak do you know how we handle configuring ingress mTLS?
There was a problem hiding this comment.
We create listener wrapped around tls server around it and pass the wrapped listener into tchannel Serve. To enable mptcp, we need to set the flags on listener in yarpc
| } | ||
|
|
||
| l, err := net.Listen("tcp", hostPort) | ||
| lc := net.ListenConfig{} |
There was a problem hiding this comment.
Note: Yarpc does not hit this method for tchannel servers, it creates a listener of its own and passes into Serve. You add this under EnableMPTCP flag
There was a problem hiding this comment.
yea I'm aware of that. But for the sake of completeness of this feature, I think we need to do this change.
There was a problem hiding this comment.
On my understanding, if a Channel is passed in as options to create a ChannelTransport, yarpc will use ListenAndServe: https://github.com/yarpc/yarpc-go/blob/dev/transport/tchannel/channel_transport.go#L168
There was a problem hiding this comment.
we actually use Transport in yarpc: https://github.com/yarpc/yarpc-go/blob/dev/transport/tchannel/transport.go#L285. It uses Serve()
4 participants
This PR adds MPTCP support. It's based on go 1.21 net package.
When EnableMPTCP is passed with ChannelOptions to create a new channel, tchannel will use MPTCP instead TCP for network connection.
MPTCP requires underlying system support MPTCP.
tchannel will use normal TCP connection if EnableMPTCP is in one of following cases: