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

[s7v7nislands]

can we expose a new function FD() return the fd?

how should we fix this in application?

1. like @tsuna ,using reflect
2. use File(), return blocked fd, then call SetNonblock()? but the comment in tcpsock.go

// The returned os.File's file descriptor is different from the
// connection's. Attempting to change properties of the original
// using this duplicate may or may not have the desired effect.

[bradfitz]

can we expose a new function FD() return the fd?

No, we're not going to do that. That exposes more than we want to expose.

[s7v7nislands]

@bradfitz So which option should I use?

I think option 2 is more nice than 1, but what the comment meaning may or may not have the desired effect?

https://www.gnu.org/software/libc/manual/html_node/File-Status-Flags.html#File-Status-Flags
in this doc, It seems will have the desired effect. right?

Sorry if I'm wrong! and Is there have another solution to set socket options?

[gopherbot]

CL https://golang.org/cl/37039 mentions this issue.

[Kubuxu]

Will https://golang.org/cl/37039 or any changes introduced in connected change-sets allow for setting of SO_REUSEPORT between socket and bind calls?

[ianlancetaylor]

@Kubuxu Not as far as I know.

[Kubuxu]

Then this issue shouldn't be marked as fixed in: https://go-review.googlesource.com/#/c/37039/

[cryptix]

thank you @ianlancetaylor for updating the thread on the go-review side.

[PhilipBorgesen]

I have development plans where I'll end up doing TCP hole punching, and ideally I would like to use the net package rather than cooking my own or using a 3rd party package. Looking through this issue however, I am unaware what the main issue is:

• Is it backwards compatible API design?
• Is it specifics to setting SO_REUSEADDR or SO_REUSEPORT?
  
  
  
  • Is it small differences in behaviour across operating systems?
  
  
• Is it implementation of other socket options than SO_REUSEADDR and SO_REUSEPORT?

In other words, what is blocking progress on this issue? What can I do to help?

[AudriusButkevicius]

So Listener is not a hard problem to solve it seems, there are quite a few libraries and they seem to do the job, and it's few tens of lines of code to make it happen.
If I am not mistaken listeners get registered with the runtime poller in net.FileListener, and no calls apart from Accept() are blocking, which is ok, as at that point the fd is already with the runtime poller.

The problem is with the dialers, as supporting timeouts for connect requires kqueue/select/epoll + ability to wake up the blocked call or tapping into the runtime poller which as it stands is impossible. Things like context support makes things even uglier.

Given we already have a concrete type net.Dialer which supports various options, would it not make sense to add an ability to specify custom flags to be passed to the kernel as part of the net.Dialer struct, before calling net.Dialer.Dial?

The API could be:

type SocketOption struct {
  Level int
  Option int
  Value interface{}
}

type Dialer struct  {
  ...
  SocketOptions SocketOption[] 
  ...
}

This way, for people trying to do TCP punch-through, we could use an external package for Listens and net.Dialer with the option in question set for dials to work around the problem.

[ianlancetaylor]

Looking this over, I think that we have a way to set options on existing sockets, via the SyscallConn method. And once https://golang.org/cl/71651 is in, we will have a way to set options on existing listening sockets. As far as I can tell, all we are missing now is a way to set options on a socket before dialing or listening.

Does anybody think we need anything else?

If so, perhaps we can add a new field to Dialer: Control func(fd uintptr) error. If non-nil this function would be called on the socket after calling setDefaultSockopts.

For listening sockets, it looks like we would need to add a new function ListenControl(network, address string, control func(fd uintptr) error). Again control would be called on the socket after calling setDefaultSockopts.

Does anybody see any case that this approach would not handle?

[rsc]

The 'fd uintptr'-ness of network connections is not otherwise exposed in package net. It would be nice to keep it that way. An alternative would be to have the new Dialer field be something like Control func(syscall.RawConn).

It seems like instead of ListenControl we should probably have an equivalent to Dialer, to plan for whatever future expansion is thrown at us, but Listener is of course taken. ListenConfig? (Probably Dialer should have been DialConfig but too late.) And then ListenConfig can have the same Control func(syscall.RawConn) field.

[AudriusButkevicius]

What @ianlancetaylor has suggested would satisfy what is being asked here.
I would also prefer Ian's API.

Given this addition is only meant to handle Control (and not Write/Read), passing RawConn adds an extra layer.

For example:

Dialer{
    Control: func(fd uintptr) error {
        return syscall.SetsockoptInt(fd, syscall.AF_INET, 0, 0)
    }
}

versus:

Dialer{
    Control: func(conn syscall.RawConn) error {
        return conn.Control(func(fd uintptr) error {
            return syscall.SetsockoptInt(fd, syscall.AF_INET, 0, 0)
        })
    }
}

[rsc]

Given this addition is only meant to handle Control (and not Write/Read), passing RawConn adds an extra layer.

Yes, exactly, and that layer avoids hard-coding in package net the system-dependent fact that an fd is a uintptr. The type uintptr does not today appear in package net's exported API, and we should keep it that way.

[AudriusButkevicius]

Yet should the attribute still be called Control given we are passing the socket as the argument, and people need to call socket.Control themselves?

[AudriusButkevicius]

So I've got most of the change ready, there are two points I'd like to discuss.

1. Whether we go with @rsc's API or @ianlancetaylor's API. I guess @rsc's makes more sense of not having to expose uintptr.
2. What functions we expose for listeners. If we go for ListenControl like suggested, to reduce copy/pasting we'd probably have to expose ListenTCPControl and ListenUnixControl. Given these two get exposed, it feels a bit strange not having it for the rest:

Listen
ListenIP
ListenMulticastUDP
ListenPacket
ListenTCP
ListenUDP
ListenUnix
ListenUnixgram

The other option would be not to expose anything for listeners, and tell people to use other libraries that already handle the listener case.

[ianlancetaylor]

Thanks for working on this. Go with @rsc's API.

This is going to be a special purpose API that few people will need to use. As far as I know you can do anything with Listen, the functions like ListenTCP are just for convenience. So for this special purpose use I think it's OK to just implement ListenControl. If that turns out to be wrong, if it turns out that we really do need ListenTCPControl, we can cross that bridge when we come to it.

[ianlancetaylor]

But I should ask--what other libraries already handle the listen case? Maybe it would be OK to just use those.

[AudriusButkevicius]

So I have not seen a generic library that allows setting options prior binding, but the most likely reason why you'd want to do this is SO_REUSEPORT, which can be solved by https://github.com/kavu/go_reuseport

[gopherbot]

Change https://golang.org/cl/72810 mentions this issue: net: allow setting socket options before bind and connect

[benburkert]

Given that all concrete connection types in the net package already have a File() (f *os.File, err error) method, would func(*os.File) error work for the new dialer field and ListenControl parameter?

[AudriusButkevicius]

The CL has been hanging for over a week now. This is my first contribution, so I am not sure how long these things take, but I'd love this to land in 1.10, and I'd definately hope that lack of comments does not imply a dead end. I guess being told no would provide a better first contribution experience than just leaving me hanging.

I've raised the issue on golang-dev as asked, but there hasn't been much feedback, as I suspect not a lot of people care about the API here.

[mikioh]

a better first contribution experience

Thanks for tackling this issue and sorry for being late; apologize for making you feel uncomfortable. Will reply to your post on golang-dev tonight.

[joeshaw]

As someone who has a need to set SO_MARK on a socket before connect(), I appreciate that the API for this includes the Dial side and not just the Listen side.

[AudriusButkevicius]

Friendly reminder.

[mikioh]

As per recent discussion at https://groups.google.com/d/msg/golang-dev/HAEaU5TakTU/ZfMLVLqEBAAJ, I think this happens in Go 1.11. Anyone who interested in this issue, feel free to review CL 72810.

[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 (https://github.com/golang/go/issues/9661#issuecomment-338285864) @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!