-
Notifications
You must be signed in to change notification settings - Fork 192
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
net: add multi listener impl for net.Listener #311
Conversation
For the testing part should I directly add an integration test that listens on net.InterfaceAddrs on multiple ports and then try to connect to server for validation? |
/cc @uablrek |
86f290e
to
66a431e
Compare
For windows, we might need a goroutine-based async multiplexer as select is not supported. |
3327c9c
to
6bd4edf
Compare
do we really need this? |
Currently, for v1alpha2 I spawn multiple helathz and metrics servers in go-routines, this could really help there. Also, Tim suggested this here |
My vote is on a library function because of testability. I can imagine some "what if"s that should be tested in (good) unit-tests at one place. I also want to emphasise my "context" comment:
|
KEP-2438 Dual-Stack API Server needs it too |
But you really shouldn't be implementing this in terms of |
0d98132
to
19a9056
Compare
net/multi_listen.go
Outdated
stopCh: make(chan struct{}), | ||
} | ||
|
||
for _, address := range addresses { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we may need to filter duplicate addresses? or do we want to fail (I think it will fail to create a listener in the same address twice)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the caller be responsible for that (not passing the same address twice).
I can't imagine a case where there is a need to listen on the same address twice, if it is passed it might be because of some bug and we should error out instead of silently ignoring the dup address, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fair
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the same area: return an error if the address slice is empty. I think it works like this now: (ml, nil) ia returned, and a subsequent Accept() hangs forever. While this is logical (sort of), it's probably not what the caller intended.
19a9056
to
ed136b1
Compare
You must have support for |
I think the multi listener must be backward compatible. That means that a call like: l,e := MultiListen(ctx, []string{":8080"}) must work, and listen on both families, and: l,e := MultiListen(ctx, []string{"0.0.0.0:8080"})
// or
l,e := MultiListen(ctx, []string{"[::]:8080"}) must also listen on both families. |
4108967
to
8cd5597
Compare
net/multi_listen.go
Outdated
_ = ml.Close() | ||
return nil, err | ||
} | ||
switch IPFamilyOf(ParseIPSloppy(host)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'd rather see this as an argument, rather than try to figure it out. It seems like an opportunity to get it wrong, when we don't need to.
net/multi_listen.go
Outdated
return ml, nil | ||
} | ||
|
||
// Accept is part of net.Listener interface. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would explain here the specific semantics - this will wait for ANY of the sub-listeners to get a connection.
net/multi_listen.go
Outdated
var _ net.Listener = &multiListener{} | ||
|
||
// MultiListen returns net.Listener which can listen for and accept | ||
// TCP connections on multiple addresses. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this comment could be more verbose to explain the semantics
net/multi_listen.go
Outdated
|
||
} | ||
|
||
// Close is part of net.Listener interface. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment, this will close ALL of the sub-listeners
The multi-listener will have different behavior compared to the standard Listener, as mentioned in #311 (comment). I can't see a way to make them exactly the same, but the differences must be investigated and documented. Otherwise users will assume that they are totally compatible. I can make some tests, but not this week. Related is how TCP backlog works in Linux. I thought I knew (I didn't), and have been hit by half-open connection problems at least once (in ~20y, so it is very rare). |
net/multi_listen_test.go
Outdated
|
||
var _ net.Listener = &fakeListener{} | ||
|
||
func TestMultiListen(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need some tests of accept-while-close, close-with-pending-connections, etc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and some tests of HTTP serving on a multi-listener
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and some tests of HTTP serving on a multi-listener
do you mean an integration test that uses real HTTP servers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it can still be a unit test (since this repo doesn't really have integration tests). Just use :0
for a random port. Create 2 or 3 such ports, pass the multi-listener to http.Serve
and then connect to each of the ports that were allocated, ensuring they get the correct HTTP response.
|
Is there any way to break a blocking Accept() (except close the Listener)?? |
OK, so... We could do:
where the semantics are basically:
with no added intelligence. If we want per-address
I think both of these are equally unambiguous, though the latter is more flexible. I can imagine that for some callers, being able to separately listen on Regardless of which of the above approaches we go to, the other question is: what should happen if you listen on both |
Taking (or building) a list of individual WRT zero addresses, if we make the caller call |
8cd5597
to
062710e
Compare
062710e
to
7c2ee8d
Compare
d6eb366
to
32b2fda
Compare
This adds an implementation of net.Listener which listens on and accepts connections from multiple addresses. Signed-off-by: Daman Arora <aroradaman@gmail.com> Co-authored-by: Tim Hockin <thockin@google.com>
32b2fda
to
27da63f
Compare
// This assumes that ANY error from Accept() will terminate the | ||
// sub-listener. We could maybe be more precise, but it | ||
// doesn't seem necessary. | ||
terminate := err != nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@thockin should I just return the error to the consumer instead of closing the listener here?
The behaviour of multiListener
will be similar to net.LIstener
if we return the error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, I think we need to return it up thru the channel so the main Accpet can return it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
/lgtm
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: aroradaman, thockin The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
This adds an implementation of net.Listener which uses synchronous multiplexing to listen on and accept connections from multiple addrs.
What type of PR is this?
/kind feature
What this PR does / why we need it:
This PR adds an implementation of net.Listener which uses synchronous multiplexing to listen to and accept connections from multiple addresses.
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Release note: