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
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@pam4
Copy link

pam4 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 the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Apr 11, 2018
@andybons andybons added this to the Unplanned milestone Apr 11, 2018
@andybons
Copy link
Member

@bradfitz

@bradfitz
Copy link
Contributor

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.

@pam4
Copy link
Author

pam4 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
Copy link
Contributor

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

Want to send the change?

@bradfitz bradfitz reopened this Apr 11, 2018
@bradfitz bradfitz added the NeedsFix The path to resolution is known, but the work has not been done. label Apr 11, 2018
@gopherbot gopherbot removed the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Apr 11, 2018
@pam4
Copy link
Author

pam4 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
Copy link

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.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

4 participants