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/http: Server panics with non-comparable Listener (because of map[net.Listener]struct{}) #24812

Closed
pam4 opened this issue Apr 11, 2018 · 6 comments

Comments

Projects
None yet
4 participants
@pam4
Copy link

commented Apr 11, 2018

https://play.golang.org/p/_O9zzZhdd-z

Usually net.Listeners are pointers, therefore comparable, but I would expect non-pointer net.Listeners to work just fine.
If my expectation is correct, perhaps this behavior should be documented.
Or the type of Server.listeners could be changed to map[*net.Listener]struct{}.

@andybons andybons added this to the Unplanned milestone Apr 11, 2018

@andybons

This comment has been minimized.

Copy link
Member

commented Apr 11, 2018

@bradfitz

This comment has been minimized.

Copy link
Member

commented Apr 11, 2018

This is true, but there's no real fix, and it doesn't seem worth documenting. Last time this came up, it seemed hypothetical at best and an audit of custom net.Listener types in the wild found they were all comparable anyway.

If we document too much, the docs become bloated and less valuable.

If we did document it somewhere, I'd do it on the net.Listener type and not in net/http at all. The net.Listener type might say "Implementations of Listener are often expected to be comparable." or something. But I'd rather not document that either.

I'm going to close this, but let me know if this actual arose in practice. That might warrant docs. But if it's just hypothetical, I'd prefer to do nothing.

@bradfitz bradfitz closed this Apr 11, 2018

@pam4

This comment has been minimized.

Copy link
Author

commented Apr 11, 2018

@bradfitz

This is true, but there's no real fix

Maybe you are right that it is not worth doing, but there is a fix.
Listeners only get in from Server.Serve, therefore the map could be a map[*net.Listener]struct{} and you could just use &l as key (l being the argument of Server.Serve).
I don't know about performance but it seems that it would require minimal change, just a few indirections.
I'm assuming that calling Server.Serve twice with the same listener is an error anyway.
Am I missing something?

I'm going to close this, but let me know if this actual arose in practice.

No, it didn't, I was just reading the code.
My thought was that merging multiple objects as anonymous fields of a struct is quite common, and if one of those is a net.Listener and another is a non-comparable type, than you have it.
Of course all you have to do is to pass the address of such struct to Server.Serve instead of the struct itself, but it may be confusing why the struct itself doesn't work.

I understand the concern about bloating the documentation, therefore if you understood my points and prefer to do nothing I'm satisfied with it.

@bradfitz

This comment has been minimized.

Copy link
Member

commented Apr 11, 2018

Oh! I thought we did more with it than we do. Storing a pointer to interface sounds fine.

Want to send the change?

@pam4

This comment has been minimized.

Copy link
Author

commented Apr 12, 2018

Thanks for your reply, I'm not ready to contribute at the moment.
If someone wants to do it before I get around to it, take a look at this git diff to see what I mean (passes tests):
https://pastebin.com/yiaK0tGY

@gopherbot

This comment has been minimized.

Copy link

commented Apr 12, 2018

Change https://golang.org/cl/106657 mentions this issue: net/http: don't crash if Server.Server is called with non-comparable Listener

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.