a new private linkage btw standard library and x/net repository

[mikioh...@gmail.com]

This is a side note for helping to review CL 37037 thru 37029, CL 37035, 30736 and 37042.

I sent the series of changes which bridges internal APIs between x/net and standard library for fixing https://github.com/golang/go/issues/19051.

The series of changes consists of two parts; changes for the standard library and changes for the x/net repository. The former provide internal APIs which are visible to the x/net repository and the latter use the APIs for manipulating inflight net.Conn and net.PacketConn safely, without worrying about the use of runtime.Finalizer and synch primitives in the net and internal/poll packages. Considering https://github.com/golang/go/issues/17244, I think it's reasonable that the internal APIs in the net package are only visible to the x/net repository for now.

 

A summary for the changes to standard library:

CL 37037, which refurbishes the required socket system calls and puts them into a new internal/socket package,

CL 37038, which glues APIs in the internal/socket package on the internal/poll package,

CL 37039, which makes internal APIs in the net package visible to the x/net repository by using the "//go:linkname" directive.

A summary for the changes to x/net repository:

CL 37035, which wraps the bridged internal APIs,

CL 37036, which fixes the breakage of ipv4 package,

CL 37042, which fixes the breakage of ipv6 package.

[mikioh...@gmail.com]

On Sunday, February 26, 2017 at 11:41:35 AM UTC+9, mikioh...@gmail.com wrote:

Considering https://github.com/golang/go/issues/17244, I think it's reasonable that the internal APIs in the net package are only visible to the x/net repository for now.

I drop the visibility control because there's a review comment suggesting that it harms vendoring; I didn't know that import path checking is automatically disabled within vendor trees.

[Ian Lance Taylor]

On Sat, Feb 25, 2017 at 6:41 PM, <mikioh...@gmail.com> wrote:
> This is a side note for helping to review CL 37037 thru 37029, CL 37035,
> 30736 and 37042.
>
> I sent the series of changes which bridges internal APIs between x/net and
> standard library for fixing https://github.com/golang/go/issues/19051.
>
> The series of changes consists of two parts; changes for the standard
> library and changes for the x/net repository. The former provide internal
> APIs which are visible to the x/net repository and the latter use the APIs
> for manipulating inflight net.Conn and net.PacketConn safely, without
> worrying about the use of runtime.Finalizer and synch primitives in the net
> and internal/poll packages. Considering
> https://github.com/golang/go/issues/17244, I think it's reasonable that the
> internal APIs in the net package are only visible to the x/net repository
> for now.
>
> A summary for the changes to standard library:
>
> CL 37037, which refurbishes the required socket system calls and puts them
> into a new internal/socket package,

I don't see the purpose of the internal/socket package. It seems like
a large amount of code duplication that will be harder to maintain in
the future. It does not make sense to me to take functions exported
by the syscall package and duplicate them into an internal package.


More generally, this e-mail describes the solution without describing
the problem. Could you describe the problem first? I don't think I
fully understand it. Once we know the problem, we'll be able to
evaluate the solution. Thanks.

Ian

[mikioh...@gmail.com]

More generally, this e-mail describes the solution without describing
the problem.  Could you describe the problem first?  I don't think I
fully understand it.  Once we know the problem, we'll be able to
evaluate the solution. 

packages x/net/{ipv4.ipv6} are basically a bundle of parsers for protocol-dependent features not supported in the standard library. the packages have used runtime reflection for scraping underlying socket descriptors from net.conn or net.packetconn, but at the same time it has known that that approach is inherently fragile because the socket descriptors are not under control of concurrency-safe access mechanisms provided by net.conn.netfd (and from go1.9, with poll.fd); under certain circumstances, for example, running descriptor duplication or quick open-close concurrently, the functionality in the packages may tread on someone else's socket descriptors.

in order to avoid the situation, i think it's better to have functions which provide a) safe access for the underlying socket descriptors thru net.conn or net.packetconn, and b) read/write access to the kernel properties without any interference from the existing syscall package; the locked-down package is designed in early day and doesn't support required features for x/net packages including support for multiple abi versions between major kernel releases.

fwiw, here is a short list of my personal requirements for the private linkage:

- provides concurrency-safe access to the protocol stack inside the kernel with the packages net and internal/poll?

- provides flexible abi views to support various protocol-dependent features and multiple kernel releases?

- provides no new exposed api?

- breaks nothing with the existing applications?

hope this helps.

[Ian Lance Taylor]

Thanks.

To me, this suggests that the internal/poll package should do the kind
of thing that the runtime package does: define an accessor function
and use go:linkname to put them in the appropriate golang.org/x/net
package. It's also OK to put the go:linkname over on the
golang.org/x/net side if that seems cleaner.

I don't understand the need for internal/socket in the standard
library. That doesn't seem to serve any purpose that the standard
library needs. If the x/net package needs more flexibility, it seems
to me that that should be in the x/net package.

Ian

[Brad Fitzpatrick]

I think any go:linkname shenanigans should point towards std, and not from std to any blessed paths outside.

I don't want "golang.org/x/net/foo" to be special and not work if it's named "vendor/golang.org/x/net/foo" or "github.com/user/x_net_fork".

--
You received this message because you are subscribed to the Google Groups "golang-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[mikioh...@gmail.com]

To me, this suggests that the internal/poll package should do the kind
of thing that the runtime package does: define an accessor function
and use go:linkname to put them in the appropriate golang.org/x/net
package.  It's also OK to put the go:linkname over on the
golang.org/x/net side if that seems cleaner.

i'm fine to stuff the internal/poll package with code fragments.

can you please explain the reason why the internal/poll is better to keep accessor functions than the net package? i think it's better  to use the exposed or builtin types as arguments for the accessor functions, not unexposed types in the internal package. am i missing something?

[Ian Lance Taylor]

I am probably missing something. I thought that you needed some sort
of internal access not available to other packages. Of course I agree
that using exposed types, and non-internal packages, for the accessor
functions is better.

Ian

[mikioh...@gmail.com]

 Of course I agree that ....

then, shall we start to reivew cl 37038-37039 and 37035, 37036, 37042 and 37417? 

[Russ Cox]

On Fri, Mar 3, 2017 at 4:03 AM, <mikioh...@gmail.com> wrote:

 Of course I agree that ....

then, shall we start to reivew cl 37038-37039 and 37035, 37036, 37042 and 37417? 

I still think this is way too complex. 

Russ

[Ian Lance Taylor]

I still don't see the purpose of the internal/socket package.

From your description of the problem, I don't see why we don't just
need to add one or two functions to the net package.

Ian

[mikioh...@gmail.com]

I still don't see the purpose of the internal/socket package.  

i dropped internal/socket package. are you talking about x/net/internal/socket package? the x/net/internal/socket is a glue package for supporting go1.8 or below and go1.9 or above.

From your description of the problem, I don't see why we don't just
need to add one or two functions to the net package. 

i'm fine either way, but i'm a bit confused. you said that "don't put anything that doesn't serve to the standard library into the standard library". if we add wrapped accessor functions to the net pacakge, the accessor functions would include a few io functions that don't serve to the standard library, for example {recv,send}mmsg and full controllable {recv,send}msg unlike the existing ones in the syscall package. does that make sense?

[Ian Lance Taylor]

On Fri, Mar 3, 2017 at 2:14 PM, <mikioh...@gmail.com> wrote:
>> I still don't see the purpose of the internal/socket package.
>
>
> i dropped internal/socket package. are you talking about
> x/net/internal/socket package? the x/net/internal/socket is a glue package
> for supporting go1.8 or below and go1.9 or above.

I'm sorry, I didn't realize that you had dropped it. I again don't
know what you are suggesting that we do.

>> From your description of the problem, I don't see why we don't just
>> need to add one or two functions to the net package.
>
>
> i'm fine either way, but i'm a bit confused. you said that "don't put
> anything that doesn't serve to the standard library into the standard
> library". if we add wrapped accessor functions to the net pacakge, the
> accessor functions would include a few io functions that don't serve to the
> standard library, for example {recv,send}mmsg and full controllable
> {recv,send}msg unlike the existing ones in the syscall package. does that
> make sense?

Sort of.

Exposing incref and decref looks like a big mess to me.

Suppose we instead add a new method on net.conn:

func (c *conn) SocketOp(f func(handle uintptr) error, read, write bool) error

This function will fetch the socket, acquire a read lock or write lock
as directed (if both are false it will call incref) and then call f
with the socket descriptor. Then you can do whatever operation you
like as long as you don't hold on to the socket descriptor.

We couldn't add this to net.Conn but it would be available using a
type assertion.

I would personally be OK with making this an exported API but I would
like to hear other opinions.

Would that help with the net package? What else would the net package need?

Ian

[Brad Fitzpatrick]

I'd love to see that as a public API.

It would reduce the number of network feature requests and bug reports and reflect chicanery a ton.

[mikioh...@gmail.com]

Exposing incref and decref looks like a big mess to me.

Suppose we instead add a new method on net.conn:

thanks, i'm fine as long as it fills my requirements. 

a nit: we cannot use net.conn because it's hard to control the visibility of methods on net.conn.

I would personally be OK with making this an exported API but I would
like to hear other opinions.

Would that help with the net package?  What else would the net package need?

just drew a rough sketch for the exposed api: https://go-review.googlesource.com/c/37813/

if it looks premature, let's use private accessor functions for go1.9 as a workaround.

[Matt Layher]

This new API looks good to me.  Thanks for doing this.  With sufficient documentation and examples, I imagine it'll be friendly enough for the users who want to make use of it.

One question: could you explain the relationship between this CL and your proposed "net/socket" package?  To me it appears that this CL focuses on providing access to the poller for existing sockets in "net", but "net/socket" would open that functionality to alternative socket types.

Thanks again. I'm thrilled to see this work happen.

- Matt

[Russ Cox]

I'm still uncomfortable with the raw-ness of this API, but a few of the rough edges would be cleaned up at least if this could be implemented as an optional interface that some net.Conns implement, along the lines of:

type RawConn interface {

// RawControl invokes f on the underlying connection's file descriptor/handle.

// The file descriptor fd is guaranteed to remain valid while f executes but not

// after f returns.

RawControl(f func(fd uintptr)) error

// RawRead invokes f on the underlying connection's file descriptor/handle;

// f is expected to try to read from the file descriptor.

// If f returns true, RawRead returns. Otherwise RawRead blocks waiting

// for the connect to be ready for reading and tries again (repeatedly).

// The file descriptor is guaranteed to remain valid while f executes but not

// after f returns.

RawRead(f func(fd uintptr) (done bool)) error

// RawWrite is like RawRead but for writing.

RawWrite(f func(fd uintptr) (done bool)) error

}

This avoids exposing the names Poller, PollOp, PolledRead, PolledWrite.

I'd still like to find a way to hide this a bit better, though. Perhaps the interface

should be syscall.Conn and the optional method on net.Conn should be

SyscallConn() (syscall.Conn, error)

That way, you can't write code that uses this unless you also import "syscall",

which should be a red flag for anyone reading or analyzing the code.

Russ

--

[mikioh...@gmail.com]

I'm still uncomfortable with the raw-ness of this API,

so, isolation could be a solution for the go team, how about having a package that taps the underlying lines between the net, internal/poll, runtime and syscall packages. for example,

- a new package net/socket defines Conn { Control(udf); Read(udf); Write(udf) } interface,

- {TCP,UDP,IP,Unix}Conn of the net package implements a method that returns a socket.Conn interface,

- an underlying type that satisfies the socket.Conn interface stays around the internal/poll package,

- the net/socket package also can be used to refurbish the existing functionality in the locked-down syscall package, to make the functionality platform-independent, as in socket control message marshaler/unmarshaler.

what do you think?

[Russ Cox]

I'm sorry, but the word "socket" nor "polled" must not appear in this API. Those are implementation details.

My proposal is:

    package syscall

    type RawConn interface {

        Control(f func(fd uintptr)) error

        Read(f func(fd uintptr) (done bool)) error

        Write(f func(fd uintptr) (done bool)) error

    }

    type Conn interface {

        SyscallConn() (syscall.RawConn, error)

    }

and then an implementation of a net.Conn may optionally implement syscall.Conn to provide access to its fd as a syscall.RawConn.

I'm confused about why you think it is preferable to add whole new packages instead of this.

Russ

--

[mikioh...@gmail.com]

I'm confused about why you think it is preferable to add whole new packages instead of this.

it's because of the docs

    "NOTE: This package is locked down. Code outside the standard Go

    repository should be migrated to use the corresponding package in the

    golang.org/x/sys repository. That is also where updates required by new

    systems or versions should be applied. See

    https://golang.org/s/go1.4-syscall for more information.

simply, i'm still not sure whether it's good to add new apis into the locked down package even if there's a rationale for it. the stated issues in go1.4-syscall are pretty right. i suppose that once we add new stuff into the syscall package it probably may start to call surrounding unordered functionality and makes the package more hard to maintain (and that's the reason why i added a few x/net packages and vendored them for supporting multiple kernel abi views instead of struggling with the syscall package.)

anyway, i'm fine with your proposal because it also meets my requirements for fixing #19051. if there's no objection i'll start to review a new series of cls based on your proposal.

--

by the way, it looks like #10565 has been blocked by the same root cause; what's the good api across over the net and syscall packages. do you have any opinion about the required api? i guess that one approach would be to make it possible to register a socket descriptor into the runtime-integrated network poller via the net package like the following:

package net

// this satisfies net.Conn, net.PacketConn and syscall.Conn interfaces.

type rawconn struct { fd *poll.FD }

func RawConn(uintptr) (Conn, error)

func RawPacketConn(uintptr) (PacketConn, error)

[Russ Cox]

On Tue, Mar 7, 2017 at 12:51 AM, <mikioh...@gmail.com> wrote:

I'm confused about why you think it is preferable to add whole new packages instead of this.

it's because of the docs

    "NOTE: This package is locked down. Code outside the standard Go

    repository should be migrated to use the corresponding package in the

    golang.org/x/sys repository. That is also where updates required by new

    systems or versions should be applied. See

    https://golang.org/s/go1.4-syscall for more information.

simply, i'm still not sure whether it's good to add new apis into the locked down package even if there's a rationale for it. the stated issues in go1.4-syscall are pretty right. i suppose that once we add new stuff into the syscall package it probably may start to call surrounding unordered functionality and makes the package more hard to maintain (and that's the reason why i added a few x/net packages and vendored them for supporting multiple kernel abi views instead of struggling with the syscall package.)

It's fine to add new API to syscall in this case, because this is about exposing new functionality from the standard library. In contrast, what we're not doing is adding every possible system call for use by code outside the standard library.

anyway, i'm fine with your proposal because it also meets my requirements for fixing #19051. if there's no objection i'll start to review a new series of cls based on your proposal.

OK, sounds good.

 

by the way, it looks like #10565 has been blocked by the same root cause; what's the good api across over the net and syscall packages. do you have any opinion about the required api? i guess that one approach would be to make it possible to register a socket descriptor into the runtime-integrated network poller via the net package like the following:

package net

// this satisfies net.Conn, net.PacketConn and syscall.Conn interfaces.

type rawconn struct { fd *poll.FD }

func RawConn(uintptr) (Conn, error)

func RawPacketConn(uintptr) (PacketConn, error)

I would still like to see syscall.RegisterSocket like in the discussions in #15021. Then plain net.FileConn can be used.

Russ

[mikioh...@gmail.com]

Mr. Layher,

>> One question: could you explain the relationship between this CL and your proposed "net/socket" package?

that's a draft "do not review" package and i have no concrete plan on it yet.

>> would open that functionality to alternative socket types.

for this topic,

I would still like to see syscall.RegisterSocket like in the discussions in #15021. Then plain net.FileConn can be used.

looks like it needs only 3 steps:

1) add RegisterSocket to the syscall package, which translates form a used-defined socket address to a syscall.Sockaddr-compliant one

2) make all socket IO functions using syscall.Sockaddr in the syscall package work properly

3) add an unexposed type that satisfies net.Conn and net.PacketConn interfaces to the net package, and make File{Conn,PacketConn} return it when appropriate

hope this helps.