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: make some way to set socket options other than using File{Listener,Conn,PacketConn} #9661

Closed
jbenet opened this issue Jan 22, 2015 · 60 comments

Comments

@jbenet
Copy link

jbenet commented Jan 22, 2015

I regret having to bring up anything related to sockets. I hate them as much as the next gopher.

There are some cases[1] where setting socket options (like SO_REUSEPORT) is just unavoidable. In most cases, we can pull out the conn.File() and set them. But those few annoying cases where the option must be set on the socket before listening or dialing are impossible to handle with today's net package. (please tell me i'm wrong!)

"Setting the socket up yourself" is not a good option. Getting sockets right is not trivial-- there are bound to be dozens of tricky corner cases across platforms. I gave it a shot, but I keep finding problems in various OSes. It would be much more user friendly to stick to the net interface which already had to do all this hard work.

Now, I would even go as far as to copy the net package wholesale just to add this feature, but AFAIK the existence of the runtime· functions makes this also impossible (such a netcopy wont compile). Without them, we're left up to whittling down netcopy (and exchanging the runtime polling for select/epoll/ugliness) , most likely introducing more bugs.

A nice way to solve this -- I think -- is to make listeners and dialers have "tuners", as implemented in https://github.com/benburkert/net.tune/ [3]. This approach is based on rob's self-referential functions, and manages to contain the ugly, exposing a nice interface.[4] But I imagine there are many other approaches that are even nicer and idiomatic.

I know-- the stdlib is trying to contain the ugly as much as possible and prevent it from spreading. But in this type of case, the current state of affairs brings it out more for some of us.


[1] One example: being able to dial out from the same TCP port you're listening on is a requirement for some kinds of NAT traversal.

[2] There's issues in this very tracker showing years of tuning the use of sockets.

[3] Specifically: https://github.com/benburkert/net.tune/blob/master/dial.go#L59-L63

[4] It could be done with new functions -- or if the goal is to keep the interface thin, a variadic argument to the existing functions should be backwards compatible.

@jbenet jbenet changed the title net: make some way to setting sockopts net: make some way to set sockopts Jan 22, 2015
@jbenet
Copy link
Author

jbenet commented Jan 22, 2015

Update: disregard this comment, the example i took it from was using select incorrectly.


Related: I think I found a "bug" in net.FileConn(f) semantics -- not sure what's expected. See: https://gist.github.com/jbenet/5c191d698fe9ec58c49d -- the outcome changes if I wait before or after the net.FileConn(f) call. this is similar to #3838. (( Maybe I'm just waiting incorrectly? I use syscall.Select because runtime_* is not avail. )) The same fix as #3838 is not easy, as raddr is not directly available.

@ianlancetaylor
Copy link
Contributor

Some of these cases can be addressed through the golang.org/x/net package. I haven't looked into this one.

@jbenet
Copy link
Author

jbenet commented Jan 22, 2015

@ianlancetaylor AFAICT, golang.org/x/net does not handle setting SO_REUSEPORT or any other options before a dial or a listen. Would love to find out i'm entirely wrong :)

@mikioh mikioh changed the title net: make some way to set sockopts net: make some way to set socket options Jan 23, 2015
jbenet added a commit to ipfs/kubo that referenced this issue Apr 8, 2015
reuseport is a hack. It is necessary for us to do certain kinds of
tcp nat traversal. Ideally, reuseport would be available in go:

  golang/go#9661

But until that issue is fixed, we're stuck with this. In some cases,
reuseport is strictly a detriment: nodes are not NATed. This commit
introduces an ENV var IPFS_REUSEPORT that can be set to false to
avoid using reuseport entirely:

  IPFS_REUSEPORT=false ipfs daemon

This approach addresses our current need. It could become a config
var if necessary. If reuseport continues to give problems, we should
look into improving it.
@jbenet
Copy link
Author

jbenet commented Apr 8, 2015

I'm curious if anybody's given thought to the approach suggested above. I wouldn't mind submitting a patch to fix this. It would make our life easier.

@rsc
Copy link
Contributor

rsc commented Apr 10, 2015

This does seem to come up a lot but we'd need to find a VERY clean way to do it.
@rsc

@coolaj86
Copy link

Not having SO_REUSEPORT is sometimes problematic. https://stackoverflow.com/questions/4465104/socket-still-listening-after-application-crash

@mikioh mikioh changed the title net: make some way to set socket options net: make some way to set socket options other than using File{Listener,Conn,PacketConn} and Socket{Conn,PacketConn} Jun 20, 2015
@mikioh
Copy link
Contributor

mikioh commented Jun 20, 2015

A random thought. There's a way to set platform, feature-specific socket options before booking like the following:

s, err := syscall.Socket(...)
if err != nil {
        // error handling
}
if err := syscall.SetsockoptInt(s, syscall.SOL_SOCKET, syscall.SO_REUSEPORT, 1); err != nil {
        syscall.Close(s)
        // error handling
}
if err := syscall.Bind(s, ...); err != nil {
        syscall.Close(s)
        // error handling
}
if err := syscall.Listen(s, ...); err != nil { // or syscall.Connect
        syscall.Close(s)
        // error handling
}
f := os.File(s, ...)
ln, err := net.FileLitsener(f) // or net.FileConn, net.FilePacketConn
f.Close()
if err != nil {
        // error handling
}
defer ln.Close() // or net.Conn.Close, net.PacketConn.Close
for {
        c, err := ln.Accept() // or net.Conn.Read/Write, net.PacketConn.ReadFrom/WriteTo
        if err != nil {
                // error handling
        }
}

and having a clean, platform and feature-agnostic way sounds pretty nice, though the reality is a bit bitter. For example, SO_REUSEPORT option makes a bit different behavior on IP control block inside the kernel between DragonFly BSD, Linux, and BSD variants, Windows. Perhaps it might annoy package net users to when they read and write data during in-service software update, networking facility on-line insertion and removal.

@mikioh
Copy link
Contributor

mikioh commented Jun 21, 2015

@jbenet,

the outcome changes if I wait before or after the net.FileConn(f)...

In general, if you don't pass an established, read/write ready socket descriptor to File{Listener,Conn,PacketConn} or Socket{Conn,PacketConn}, you need to understand the entire behavior of event IO mechanism, epoll, kqueue or Windows IOCP, which is used in runtime-integrated network poller.

@jbenet
Copy link
Author

jbenet commented Jun 22, 2015

@mikioh thanks, yep.

runtime-integrated network poller.

Since the runtime-integrated network poller came up, i think it should be exposed in some way for users to build pkgs/libs with it. Making the stdlib net package privileged in a way no other network package could ever be:

  • makes problems like this lack of SO_REUSEPORT for TCP/UDP harder to solve,
  • harms other networking libraries which may need to manipulate sockets at lower levels than net package interfaces allow,
  • harms implementations of other protocols (e.g. SCTP, uTP, QUIC),
  • and is not clean.

This is a separate issue altogether-- if you agree with the above claims, I can file another issue and expand on the problem.

@mikioh
Copy link
Contributor

mikioh commented Jun 22, 2015

@jbenet,

Please take a look at https://groups.google.com/d/msg/golang-dev/4GOiSBCX568/brrg9aCbqngJ, speak up for the ongoing development process and follow the procedure If you have some proposals. I have no concrete opinions on both issues at the moment.

@mikioh mikioh changed the title net: make some way to set socket options other than using File{Listener,Conn,PacketConn} and Socket{Conn,PacketConn} net: make some way to set socket options other than using File{Listener,Conn,PacketConn} Jul 2, 2015
jbenet added a commit to libp2p/go-libp2p that referenced this issue Sep 30, 2015
reuseport is a hack. It is necessary for us to do certain kinds of
tcp nat traversal. Ideally, reuseport would be available in go:

  golang/go#9661

But until that issue is fixed, we're stuck with this. In some cases,
reuseport is strictly a detriment: nodes are not NATed. This commit
introduces an ENV var IPFS_REUSEPORT that can be set to false to
avoid using reuseport entirely:

  IPFS_REUSEPORT=false ipfs daemon

This approach addresses our current need. It could become a config
var if necessary. If reuseport continues to give problems, we should
look into improving it.
@tsuna
Copy link
Contributor

tsuna commented Jan 24, 2016

Another case where this is a problem is when one wants to set IP_TOS (for DSCP) on a socket before accepting any connection. The more verbose solution proposed above by @mikioh is fraught with peril, as there are many minutiae that needs to be attended and that vary greatly across platforms (how to set the FD non-blocking, close-on-exec, how to find a decent default backlog value to pass to listen, etc).

I also went down a similar path as @jbenet and ended up replicating a simplified version of a bunch of code from the net package, but it's really not a tractable solution.

+1 to the proposal of using function options to allow the user to set tunables.

@tsuna
Copy link
Contributor

tsuna commented Jan 25, 2016

I ended up using reflection to get a hold of the FD after calling ListenTCP. This was a lot less code.

https://github.com/aristanetworks/goarista/blob/master/dscp/listen_unix.go

@jbenet
Copy link
Author

jbenet commented Feb 3, 2016

@tsuna does that work cross-arch? dont recall atm, but I think this is still not a solution for SO_REUSEPORT as it has to be set before, else a listen will fail.

@tsuna
Copy link
Contributor

tsuna commented Feb 3, 2016

This works for the UNIX implementation of netFD, so anything but Windows and Plan9. This kludge does not set the flag before the listen() system call is issued, unfortunately. It worked for me to set IP_TOS, but if SO_REUSEPORT needs to be set before calling listen() then I'm sorry for the noise.

Exact sequence of events that happen when using my kludge to set the TOS to 40 (as per strace on Linux):

socket(PF_INET, SOCK_STREAM|SOCK_CLOEXEC|SOCK_NONBLOCK, IPPROTO_IP) = 10
setsockopt(10, SOL_SOCKET, SO_BROADCAST, [1], 4) = 0
setsockopt(10, SOL_SOCKET, SO_REUSEADDR, [1], 4) = 0
bind(10, {sa_family=AF_INET, sin_port=htons(6042), sin_addr=inet_addr("127.0.0.1")}, 16) = 0
listen(10, 128)                   = 0        // <-------------
epoll_ctl(8, EPOLL_CTL_ADD, 10, {...}) = 0
getsockname(10, {sa_family=AF_INET, sin_port=htons(6042), sin_addr=inet_addr("127.0.0.1")}, [16]) = 0
setsockopt(10, SOL_IP, IP_TOS, [40], 4) = 0  // <-------------
// ...
accept4(10, 0x18fa6ca8, [112], SOCK_CLOEXEC|SOCK_NONBLOCK) = -1 EAGAIN (Resource temporarily unavailable)

@nehbit
Copy link

nehbit commented May 24, 2018

Shamelessly pinging this issue on behalf of @AudriusButkevicius. I think this one's been waiting for a lgtm and/or review. This is quite a useful feature because it helps a lot in implementing efficient NAT hole punching.

@AudriusButkevicius
Copy link
Contributor

AudriusButkevicius commented May 28, 2018

I am sure everyone is busy and all, but to leave a CL hanging for over a month, without any indication on what it is waiting for is not ok. Drop a quick note letting us know that it will be reviewed in the future or that it's not the right implementation, or whatever. Any response is better than no response.

So far my attempt to contribute to Go has been a very disappointing experience.

I am surprised people have the patience to wait in silence for whatever it is we are waiting for in order to contribute.

@ashi009
Copy link

ashi009 commented May 28, 2018

Hi @AudriusButkevicius, it’s normal for a cl that touches core library to go through a extra thorough review process and it might take months.

Judging by your cl, it still has 13 unresolved comments. Until they are all resolved, your cl won’t pop up to the front of the reviewers attention list. That’s probably the reason why it’s not getting any updates.

@AudriusButkevicius
Copy link
Contributor

I've resolved them now, but I was expecting that a new patch set would dismiss them.
Anyways, it would have been nice if someone after me repeatedly bumping the CL would have told me a month ago that the ball is still in my court, or gerrit UI could do that.

@ashi009
Copy link

ashi009 commented May 28, 2018

You also need to rebase it on top the master, as there are merge confilcts.

@gopherbot
Copy link

Change https://golang.org/cl/114827 mentions this issue: net: allow setting socket options before listening or dialing

@ashi009
Copy link

ashi009 commented May 29, 2018

@AudriusButkevicius I just take another look at your CL, and I don't think you should resolve those comments as you didn't actually address to them. All the review comments are mandatory unless you have a concrete reason against it. Please revisit those comments, and push a new patchset, then reply with PTAL ("please take another look") in Gerrit.

@AudriusButkevicius
Copy link
Contributor

Even if the comments are no longee relevant and the code is gone?

@ashi009
Copy link

ashi009 commented May 29, 2018

@AudriusButkevicius
Copy link
Contributor

Sorry, I am completely lost. The first one was addressed in patch 7, the other two which were left on patch 3 were addresses in patch 5. Are you saying they have to marked as done by the person who made the original comment?

@ashi009
Copy link

ashi009 commented May 29, 2018

Oops, apparently, I was mistakenly thought the link was pointing to the latest patchset and didn't see the later fixes.

I'm totally sorry about this, please ignore my comments and just wait for the reviewer to start another round of review. Looking forward to seeing your CL merged.

@AudriusButkevicius
Copy link
Contributor

There are two comments that are not addressed but that is because they need some discussion. Do I need to ack them after replying or just reply and wait?

@ashi009
Copy link

ashi009 commented May 29, 2018

You should mark them as resolved, as you have already answered the questions. If the reviewer is not satisfied, they will reopen the comment.

Otherwise, they will consider it's still your turn (I think Gerrit still has the issue of not being able to detect "whose the next turn").

@ianlancetaylor
Copy link
Contributor

The CL (https://golang.org/cl/72810) introduces a new type net.Announcer. This type stands in relation to net.Listen as net.Dialer does to net.Dial. We can't name this new type Listener, as net.Listener already exists as an interface type.

I can't say that I'm crazy about the name Announcer. Above (#9661 (comment)) @rsc suggested ListenConfig which is not great but at least conveys the basic idea: a more configurable Listener.

Any other suggestions?

@ianlancetaylor
Copy link
Contributor

Another issue: the new Announcer.Listen method gives us the opportunity to add a context.Context argument, rather than using context.Background. Any reason not to do that?

@AudriusButkevicius
Copy link
Contributor

AudriusButkevicius commented May 29, 2018

I guess I prefer ListenConfig over Announcer.

I don't have any objections in regards to adding context usage for the Listen method, yet I've asked a question on the CL as to how the API should look like.

ListenConfig.Listen(ctx, net, addr) vs ListenConfig.ListenContext(ctx, net, addr)

where the latter matches Dialer.DialContext.

Perhaps we can have both, just like the dialer,

ListenConfig.ListenContext(ctx, net, addr)
ListenConfig.Listen(net, addr)

@ianlancetaylor
Copy link
Contributor

We only have both for Dialer for historical reasons, since Dialer was developed before the context package existed.

My vote would be to call the method Listen rather than ListenContext, even though it takes a Context parameter.

@gopherbot
Copy link

Change https://golang.org/cl/115175 mentions this issue: net: move dial and listen functions under sysDialer, sysListener

gopherbot pushed a commit that referenced this issue May 29, 2018
Updates #9661

Change-Id: I237e7502cb9faad6dece1e25b1a503739c54d826
Reviewed-on: https://go-review.googlesource.com/115175
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@whyrusleeping
Copy link

Thanks everyone!

@nicola
Copy link

nicola commented May 31, 2018

woooohoooo!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests