Skip to content
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: listening on a local port for a test requires tedious boilerplate #24778

Closed
glasser opened this issue Apr 9, 2018 · 7 comments
Closed
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone

Comments

@glasser
Copy link
Contributor

glasser commented Apr 9, 2018

In the stdlib alone, there are 6 functions called newLocalListener that do something roughly like

	l, err := net.Listen("tcp", "127.0.0.1:0")
	if err != nil {
		if l, err = net.Listen("tcp6", "[::1]:0"); err != nil {
			panic(fmt.Sprintf("httptest: failed to listen on a port: %v", err))
		}
	}
	return l

(there are admittedly differences in between them in how they handle errors, etc)

It would be nice (for consumers of the stdlib as well) if there was an easy way to do this with one line.

You might think that listening on the unspecified interface would help, but there are two downsides. First, you might not want your test server to be externally accessible! Second, if you listen on the unspecified interface then your listener.Addr() is like "0.0.0.0" or "::" which you may not be able to connect to in all environments (and you may run into issues like #24737 as well).

(Arguably this would go well in a net/nettest package like net/http/httptest, but that isn't a thing.)

@neild
Copy link
Contributor

neild commented Apr 9, 2018

I presume there's a reason to not use net.Listen("tcp", "localhost:0"), but I'm not certain what it is.

@neild
Copy link
Contributor

neild commented Apr 9, 2018

...oh, net.Listen requires that you specify "tcp" or "tcp6". That would be the reason. :)

@bradfitz
Copy link
Contributor

bradfitz commented Apr 9, 2018

Perhaps in the spirit of https://tools.ietf.org/html/draft-west-let-localhost-be-localhost-06 we should special case a lookup of "localhost" when the network is "tcp" (not "tcp4" or "tcp6") and just do the right thing, rather than adding any new API.

That is, if your system's DNS says that "localhost" is "127.0.0.1" and you're on an IPv6-only system, then Go shouldn't care about your DNS value if you say net.Listen("tcp", "localhost:0") and we should instead listen on ::1, because that's what you meant by "localhost".

/cc @mikioh @ianlancetaylor

@bradfitz bradfitz added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Apr 9, 2018
@bradfitz bradfitz added this to the Go1.11 milestone Apr 9, 2018
@neild
Copy link
Contributor

neild commented Apr 9, 2018

Even without special casing, I believe "localhost" should do the right thing on any properly configured system. My /etc/hosts contains localhost entries for both 127.0.0.1 and ::1, and net.Listen("tcp", "localhost:0") behaves reasonably if I comment out one or the other.

(My previous comment about "tcp" vs. "tcp6" is wrong; "tcp" happily resolves either v4 or v6 addresses.)

@bradfitz
Copy link
Contributor

bradfitz commented Apr 9, 2018

I believe "localhost" should do the right thing on any properly configured system

My proposal is we let localhost be localhost regardless of their config.

@rsc
Copy link
Contributor

rsc commented Apr 9, 2018

Closing as dup of #22826 but it seems like a good idea, provided we can get some insight into how close the draft is to being an RFC. Note that the draft in this issue is from August 2017 while the draft in the other issue is newer, from December 2017.

@rsc rsc closed this as completed Apr 9, 2018
@neild
Copy link
Contributor

neild commented Apr 9, 2018

#22826 (let localhost be localhost) SGTM, but I think it's reasonable to use net.Listen("tcp", "localhost:0") before we do that or even if we don't.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
None yet
Development

No branches or pull requests

5 participants