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

proposal: net: add SyscallConn for minor protocols #28144

Closed
mikioh opened this issue Oct 11, 2018 · 25 comments

Comments

Projects
None yet
6 participants
@mikioh
Copy link
Contributor

commented Oct 11, 2018

This is a supplementary proposal for #15021, also a fix for #10565 and its variants.

If I understand correctly, the requirements described in the issues are simply like the following:
R1: accommodate various protocol- or platform-dependent connection setup, read and write system calls
R2: accommodate various protocol-dependent data, mostly protocol address structures
R3: make a pair of R1 and R2 work together with the runtime-integrated network poller

Unfortunately, #15021 just covers only R2 and R3 even though it prevents #22191 from moving forward. As far as I can see, common SCTP implementations provide a bit quaint system calls for its multipath-capable connection setup and data transmission. In addition, MPTCP and TCP fast open support on Darwin also takes the same approach; see sa_endpoints_t, sae_connid_t and connectx.

To cover all of the requirements, I propose to add a new struct type to the net package for turning user socket descriptors into the runtime-integrated network poller without supporting various system calls and data structures in the standard library.

New exposed API:
pkg net, func NewSyscallConn(string, *os.File) (*SyscallConn, error)
pkg net, method (*SyscallConn) Close() error
pkg net, method (*SyscallConn) SetDeadline(time.Time) error
pkg net, method (*SyscallConn) SetReadDeadline(time.Time) error
pkg net, method (*SyscallConn) SetWriteDeadline(time.Time) error
pkg net, method (*SyscallConn) SyscallConn() (syscall.RawConn, error)
pkg net, type SyscallConn struct

See https://go-review.googlesource.com/c/go/+/141437 for details

The struct type has a method returning a syscall.RawConn so that users are able to implement any system call with any protocol-dependent data structure without worrying about the development speed of the standard library and x/net or x/sys repository. In addition, fixing #15021 in the x/sys repository would be a great help and this proposal would work well together with the fix for #15021 on x/sys repository.

@gopherbot gopherbot added this to the Proposal milestone Oct 11, 2018

@gopherbot gopherbot added the Proposal label Oct 11, 2018

@acln0

This comment has been minimized.

Copy link
Contributor

commented Oct 11, 2018

Thank you for sending the proposal and moving things forward.

Indeed, if the required system calls and data structures don't match the ones used by the net and internal/poll package, using the new API implemented by CL 141437 is difficult. This solution is much simpler, and can cover more cases with ease.

In addition, fixing #15021 in the x/sys repository would be a great help and this proposal would work well together with the fix for #15021 on x/sys repository.

I'm a little bit confused about this. If this proposal were to be accepted, what do we need the registry for? And if we would have it, what kind of mappings would it contain? The registry was going to be used by package net to implement net.Conn and net.PacketConn, particularly for dealing with addresses. But in this case, callers can do it themselves, because x/sys/unix has its own Sockaddr interface, and it's easy to add new types if needed.

I know it's pretty late in the cycle, but I would very much like to find a resolution for #15021 and similar issues, for Go 1.12. I am also willing to do whatever work might be necessary.

@mikioh

This comment has been minimized.

Copy link
Contributor Author

commented Oct 12, 2018

what do we need the registry for?

It should not be a mandatory option, but we still see issues like "the net package doesn't provide a socket something-something I really need (or want)", perhaps there might be an option for the sake of convenience in the x/sys repository.

what kind of mappings would it contain?

That's a good question. Sorry, I don't have any concrete idea yet. I enjoyed an idea of replacing the existing unix.Sockaddr interface and system call wrapper functions with something using generic facilities (interface and parametric polymorphism are orthogonal, sounds neat), but the lack of operational experiences (e.g., how to manage the distribution of contracts; along with the per-package basis under cmd/go control or having a dedicated package?) prevented me from moving forward.

So, I mean, this issue smashes the necessity of #15021 but there perhaps might be a reason for keeping #15021 open.

@rsc

This comment has been minimized.

Copy link
Contributor

commented Oct 17, 2018

How is the proposed net.NewSyscallConn function different from the net.FileConn function?
Given that we already have net.FileConn, it seems like we should keep using that.
Is there something it doesn't do well enough?

@acln0

This comment has been minimized.

Copy link
Contributor

commented Oct 17, 2018

Currently, net.FileConn and net.FilePacketConn only work if the file descriptor refers to a socket that the net package knows how deal with. It would be nice if we could use other sockets through the runtime network poller. Using a non-blocking os.File is generally not sufficient, because in certain cases, we can't make the right system calls while making use of the poller.

The proposed net.SyscallConn would help with this issue. Callers could wrap it, and use its attached syscall.RawConn to make system calls such as recvfrom(2) for I/O, while the underlying file descriptor is registered with the poller.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Oct 17, 2018

Can you clarify why a non-blocking os.File doesn't work? Which system calls do you need to make that are not made today?

Also, can you clarify what you mea by "a socket that the net package knows how to deal with?"

Am I correct in thinking that this is all about the runtime poller? We've been working toward having all non-blocking descriptors use the poller. It's not clear to me why we need to introduce a new type here. There may be some clear reason, but I don't see it.

@mikioh

This comment has been minimized.

Copy link
Contributor Author

commented Oct 17, 2018

different from the net.FileConn function?

Simply the series of File{Conn,PacketConn,Listener} is designed for Unix variants and doesn't work well on Windows. There was (is?) a few attempts to implement the series of File API on Windows but didn't succeed. On Windows, the duplication and inheritance of socket handles behave a bit different from Unix variants and the attempts failed to pass the existing tests, IIRC.

NewSyscallConn doesn't do socket descriptor or handle duplication.

@mikioh

This comment has been minimized.

Copy link
Contributor Author

commented Oct 17, 2018

There may be some clear reason, but I don't see it.

To allow users to implement protocol- or platform-dependent system calls like http://man7.org/linux/man-pages/man3/sctp_send.3.html.

@acln0

This comment has been minimized.

Copy link
Contributor

commented Oct 17, 2018

Which system calls do you need to make that are not made today?

For instance, recvmsg on a netlink socket, with the help of the poller. Perhaps I am missing something, but I don't see how that could be done today.

Also, can you clarify what you mean by "a socket that the net package knows how to deal with?"

For example, a netlink or SCTP socket. The net package does not support such sockets, and net.FileConn and net.FilePacketConn fail when passed such a file descriptor.

Am I correct in thinking that this is all about the runtime poller?

Yes, pretty much. It is about being able to execute system calls in the context of a syscall.RawConn, on a file descriptor that has been registered with the poller. As far as I can tell, this is currently not possible.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Oct 17, 2018

Simply the series of File{Conn,PacketConn,Listener} is designed for Unix variants and doesn't work well on Windows. There was (is?) a few attempts to implement the series of File API on Windows but didn't succeed. On Windows, the duplication and inheritance of socket handles behave a bit different from Unix variants and the attempts failed to pass the existing tests, IIRC.

NewSyscallConn doesn't do socket descriptor or handle duplication.

Can we make FileConn work on Windows? What prevents that from happening? Right now net.FileConn looks a lot like your proposed net.NewSyscallConn. What's the difference?

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Oct 17, 2018

For instance, recvmsg on a netlink socket, with the help of the poller. Perhaps I am missing something, but I don't see how that could be done today.

The idea would be to use SyscallConn method to get a syscall.RawConn and call the Read method.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Oct 17, 2018

The net package does not support such sockets, and net.FileConn and net.FilePacketConn fail when passed such a file descriptor.

How do they fail?

Sorry for asking dumb questions, but this code is already really complicated, and this proposal does not have the background information explaining why we need to take this approach.

@acln0

This comment has been minimized.

Copy link
Contributor

commented Oct 17, 2018

They fail like so: https://github.com/golang/go/blob/master/src/net/file_unix.go#L41-L53

This is called by both the FileConn and FilePacketConn code paths.

@mikioh

This comment has been minimized.

Copy link
Contributor Author

commented Oct 17, 2018

Can we make FileConn work on Windows?

Sorry, I'm not a Windows user and have no Windows stuff; see https://go-review.googlesource.com/c/go/+/8683

@acln0

This comment has been minimized.

Copy link
Contributor

commented Oct 17, 2018

Alternative problem statement: there is currently no way to obtain, for an arbitrary file descriptor, an implementation of syscall.RawConn such that the associated file descriptor is registered with the poller.

@mikioh

This comment has been minimized.

Copy link
Contributor Author

commented Oct 17, 2018

@acln0,

Alternative problem statement

That should be another proposal or issue; please file a new issue if you can address the issue.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Oct 31, 2018

What if we change newFileFD to ask the socket for the family, and carry on rather than returning EPROTONOSUPPORT?

Can somebody share a small piece of code that ought to work but doesn't, using net.FileConn? Thanks.

@acln0

This comment has been minimized.

Copy link
Contributor

commented Nov 1, 2018

This example uses net.FilePacketConn, rather than net.FileConn, but hopefully it demonstrates the issue.

https://play.golang.org/p/HQHSKBfJIl2

I'm not convinced that this code ought to work. Assume newFileFD carries on if the socket type and family don't match one of the socket types supported by package net directly, which is the case in the example code above. What implementation of net.PacketConn should FilePacketConn return?

One option is to return an implementation of PacketConn such that its ReadFrom and WriteTo always return errors, and its LocalAddr returns nil (or some other token value). The example code assumes this is the case. This seems suspicious: the net package returns a PacketConn, but 3 of its methods don't work, and all the interesting work has to be done using syscall.RawConn anyway. (note: the method set of the proposed net.SyscallConn contains exactly the methods which would work on this hypothetical implementation of PacketConn)

An alternative is to make FilePacketConn return an implementation of PacketConn such that its ReadFrom, WriteTo and LocalAddr methods actually work. In order to do that, we would need a way for the net package to map net.Addr values to syscall.Sockaddr values of the appropriate type, and vice-versa. That is what https://go-review.googlesource.com/c/go/+/136595 tries to do, but the approach there was deemed ineffective. Indeed, it seems like instead of making package net and package syscall manage all the complexities of string <-> syscall.Sockaddr mappings, it would be easier to let callers deal with addresses themselves. If callers can handle addresses, it opens up a lot of new possibilities, because callers can use x/sys/unix rather than the frozen syscall package.

All in all, I believe the proposed net.SyscallConn offers the most straight-forward, extensible solution to the problem. Yes, it does unfortunately introduce new API, but it avoids both dysfunctional implementations of interfaces, and adding additional internal complexity to package net.

@crvv

This comment has been minimized.

Copy link
Contributor

commented Nov 1, 2018

New exposed API:
pkg net, func NewSyscallConn(string, *os.File) (*SyscallConn, error)
pkg net, method (*SyscallConn) Close() error
pkg net, method (*SyscallConn) SetDeadline(time.Time) error
pkg net, method (*SyscallConn) SetReadDeadline(time.Time) error
pkg net, method (*SyscallConn) SetWriteDeadline(time.Time) error
pkg net, method (*SyscallConn) SyscallConn() (syscall.RawConn, error)
pkg net, type SyscallConn struct

The new API turns an *os.File into a *net.SyscallConn

The first 4 methods are Close, SetDeadline, SetReadDeadline and SetWriteDeadline.
*os.File has these methods so they are not important.

The last method SyscallConn is the point of the proposal.
It turns an *os.File into a syscall.RawConn.

As said before by @acln0

Alternative problem statement: there is currently no way to obtain, for an arbitrary file descriptor, an implementation of syscall.RawConn such that the associated file descriptor is registered with the poller.

I don't think this is an "alternative problem". This is the problem that the proposal try to solve.

With Go1.11, you can use an arbitrary fd to obtain an *os.File registered with the poller.
What you can't do is the next step, obtaining a syscall.RawConn

@acln0

This comment has been minimized.

Copy link
Contributor

commented Nov 1, 2018

Sorry. Language barrier. I wanted "alternative problem statement" to mean "a different way of phrasing the problem".

@rsc

This comment has been minimized.

Copy link
Contributor

commented Dec 12, 2018

Sorry for the long conversation. I am still a little confused. The net package looks for specific socket families so that it can be sure it knows the Go type to return from net.Conn's LocalAddr and RemoteAddr methods. What should it return if it doesn't know the family?

If you don't need LocalAddr and RemoteAddr, all that's left in net.Conn is Read, Write, Close, SetDeadline, SetReadDeadline, and SetWriteDeadline. Those are all now present in os.File. So if you don't need the addresses, it seems like you can now use an os.File directly. If you do need the addresses, how should they behave?

Also, if we do need some way to provide hooks into LocalAddr and RemoteAddr, isn't that #15021?

@acln0

This comment has been minimized.

Copy link
Contributor

commented Dec 17, 2018

@rsc

My understanding is that the fundamental problem which both #15021 and this bug try to solve is the following: we would like to perform system calls on socket file descriptors with the help of the runtime poller, and these system calls are not limited to Read and Write (which is why an os.File is not enough). For example, this netlink library calls recvmsg: https://github.com/mdlayher/netlink/blob/master/conn_linux.go#L140. When timeouts are needed for such calls, solutions such as vishvananda/netlink@17ea11b appear, which seem unpleasant and almost wrong. Go has a poller built-in. We should be able to make use of it.

The current solution to the problem of "I want to use the poller when making a system call on the file descriptor of a socket" is to use syscall.RawConn, which is implemented in package net for the types of sockets the net package knows about. But there is no implementation of syscall.RawConn (one which makes use of the runtime poller) for socket types the net package does not know about directly.

#15021 argues that the solution is to change the net package to offer some mechanism to register hooks into LocalAddr and RemoteAddr. This would answer the question of "What should it return if it doesn't know the family?". Using this mechanism, the net package would wrap sockets it doesn't (directly) know about in Conn or PacketConn implementations, via FileConn or FilePacketConn. Those implementations would use the network poller for the usual system calls, and would also implement syscall.RawConn for completeness. A demo of this was presented in https://go-review.googlesource.com/c/go/+/45790/, which I implemented in https://go-review.googlesource.com/c/go/+/136595. The approach in my CL was rightfully deemed ineffective, and this bug was filed instead, as a better solution for the problem.

The point is, when callers want to make a specific system call on the file descriptor, they're going to want to use syscall.RawConn anyway, instead of the net.Conn or net.PacketConn methods. Instead of encumbering the net and syscall packages with managing hooks for addresses, this proposal argues that the net package should be changed to offer what I would call a minimal syscall.RawConn implementation. Managing addresses and choosing system calls remains entirely up to the caller. The caller can now implement net.Conn or net.PacketConn effectively, if they need to, because the new net.SyscallConn offers a syscall.RawConn which uses the poller.

In conclusion, I think #15021 doesn't focus on the correct fundamental issue, because it gets bogged down in the address problem which your message alludes to. I think the addresses are a red herring. The key is syscall.RawConn. What this proposal asks for is a tiny wrapper around a poll.FD, which implements syscall.RawConn, because that is the minimal API to allow callers to implement anything else they would like in terms of it.

@rsc

This comment has been minimized.

Copy link
Contributor

commented Dec 19, 2018

Sorry, there's a lot to digest in your response. If we did submit your CL 136595, would that address the problem? You say it was rightly deemed ineffective but all I see is Mikio saying "looks like your approach cannot support sctp-like multipath transport protocols perfectly." That's not quite "ineffective" to me, just that there are even more obscure corner cases remaining.

We really don't want to expose syscall.RawConn more than we already have.

@mikioh

This comment has been minimized.

Copy link
Contributor Author

commented Dec 20, 2018

Hi all,

I'm a bit hard to recognize who is "you." Probably it's better to add identifiers for distinction. Looks like this issue now has three streams:

  1. src=@rsc,@ianlancetaylor payload="we don't understand this issue because we created net.FileConn for the purpose of file descriptor passing"
  2. src=@crvv payload="the kernel of this issue is to obtain a syscall.RawConn"
  3. src=@acln0 payload="... (hm, still not sure but perhaps the same as (2) with more details)"

Is my understanding (roughly) correct? If so, I will add a few comments to bridge the gap between (1) and (2)+(3) till the end of this year.

@crvv

This comment has been minimized.

Copy link
Contributor

commented Dec 20, 2018

There is another issue for adding (*os.File).SyscallConn, #24331
That method does the same thing through a different route.
I think only one of these functions is required.

@mikioh

This comment has been minimized.

Copy link
Contributor Author

commented Dec 28, 2018

Looks like #24331 covers the requirements R1, R2 and R3, so I'll merge this issue to #24331.

@mikioh mikioh closed this Dec 28, 2018

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