golang / go

The Go programming language
https://go.dev
BSD 3-Clause "New" or "Revised" License
123.55k stars 17.61k forks source link

proposal: x/net/netutil: export netutil.LimitListenerConn #64830

Open zmt opened 10 months ago

zmt commented 10 months ago

Go version

go1.21.5

What operating system and processor architecture are you using (go env)?

% go env
GO111MODULE='on'
GOARCH='amd64'
GOBIN=''
GOCACHE='/home/user/.cache/go-build'
GOENV='/home/user/.config/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='amd64'
GOHOSTOS='linux'
GOINSECURE=''
GOMODCACHE='/home/user/gocode/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='linux'
GOPATH='/home/user/gocode'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/home/user/gocode/src/github.com/spiffe/spire/.build/linux-x86_64/go/1.21.5'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/home/user/gocode/src/github.com/spiffe/spire/.build/linux-x86_64/go/1.21.5/pkg/tool/linux_amd64'
GOVCS=''
GOVERSION='go1.21.5'
GCCGO='gccgo'
GOAMD64='v1'
AR='ar'
CC='gcc'
CXX='g++'
CGO_ENABLED='1'
GOMOD='/home/user/gocode/src/github.com/spiffe/spire/go.mod'
GOWORK=''
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
PKG_CONFIG='pkg-config'
GOGCCFLAGS='-fPIC -m64 -pthread -Wl,--no-gc-sections -fmessage-length=0 -ffile-prefix-map=/tmp/go-build1677547490=/tmp/go-build -gno-record-gcc-switches'

What did you do?

I was attempting to use the https://pkg.go.dev/golang.org/x/net/netutil#LimitListener to wrap the *net.UnixListener in listener_posix.go to limit connection acceptance at a theshold caclulated as a percentage of https://pkg.go.dev/syscall#Rlimit. The peertracker code needs to assert the underlying *net.UnixConn type here.

I tested out a proposed fix using a local filesystem replace directive to code with this patch:

% git diff -p HEAD~
diff --git a/netutil/listen.go b/netutil/listen.go
index f8b779e..5caf94e 100644
--- a/netutil/listen.go
+++ b/netutil/listen.go
@@ -65,7 +65,7 @@ func (l *limitListener) Accept() (net.Conn, error) {
                l.release()
                return nil, err
        }
-       return &limitListenerConn{Conn: c, release: l.release}, nil
+       return &LimitListenerConn{Conn: c, release: l.release}, nil
 }

 func (l *limitListener) Close() error {
@@ -74,13 +74,13 @@ func (l *limitListener) Close() error {
        return err
 }

-type limitListenerConn struct {
+type LimitListenerConn struct {
        net.Conn
        releaseOnce sync.Once
        release     func()
 }

-func (l *limitListenerConn) Close() error {
+func (l *LimitListenerConn) Close() error {
        err := l.Conn.Close()
        l.releaseOnce.Do(l.release)
        return err

What did you expect to see?

I expected to be able to get at the underlying *net.UnixConn after wrapping with the netutil.LimitListener.

What did you see instead?

The limitListenerConn was unexported so I couldn't assert the type of the embedded net.Conn.

thanm commented 10 months ago

@neild @ianlancetaylor per owners.

It sounds as though you are asking for a change to the exported API of the net package, which is typically something that would require proposal review (see proposing changes.

zmt commented 10 months ago

@neild @ianlancetaylor per owners.

It sounds as though you are asking for a change to the exported API of the net package, which is typically something that would require proposal review (see proposing changes.

Strictly speaking, yes, it is a change to the exported API. However, it is in the netutil package from the golang.org/x/net module. My understanding is that /x path prefix indicates "experimental". I wasn't clear if that would require a formal proposal to change an experimental package, so I started with a simple issue, including the proposed patch content.

zmt commented 10 months ago

@thanm from the proposing changes:

Note: A non-proposal issue can be turned into a proposal by simply adding the proposal label.

Please add the label if you think it is appropriate. I am unable to do so myself.