net: make some way to set socket options other than using File{Listener,Conn,PacketConn}

[jbenet]

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]

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]

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

[jbenet]

@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 :)

[jbenet]

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]

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

[coolaj86]

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

[mikioh]

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]

@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]

@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]

@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.

[tsuna]

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]

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]

@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]

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]

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]

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]

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]

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]

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

[gopherbot]

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

[ashi009]

@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]

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

[ashi009]

For that case, it's OK to mark it as done. But there are many needs to be addressed, just some random picks:

• https://go-review.googlesource.com/c/go/+/72810/6//COMMIT_MSG#7
• https://go-review.googlesource.com/c/go/+/72810/3/src/net/ipsock_posix.go#136
• https://go-review.googlesource.com/c/go/+/72810/3/src/net/dial_test.go#916

[AudriusButkevicius]

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]

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]

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]

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]

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]

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]

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]

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]

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

[whyrusleeping]

Thanks everyone!

[nicola]

woooohoooo!