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.

Sign up for GitHub

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

Jump to bottom

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

Open
zmt opened this issue Dec 21, 2023 · 3 comments
Open

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

zmt opened this issue Dec 21, 2023 · 3 comments
Labels
Proposal
Milestone
Proposal

Comments

@zmt
Copy link

zmt commented Dec 21, 2023 •
edited

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.
@gopherbot gopherbot added this to the Unreleased milestone Dec 21, 2023
@zmt zmt mentioned this issue Dec 21, 2023
Closed
3 tasks
@thanm thanm added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Dec 21, 2023
@thanm
Copy link
Contributor

thanm commented Dec 21, 2023

@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
Copy link
Author

zmt commented Dec 21, 2023

@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
Copy link
Author

zmt commented Dec 21, 2023

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

@seankhliao seankhliao added Proposal and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Dec 22, 2023
@seankhliao seankhliao changed the title x/net: export netutil.LimitListenerConn proposal: x/net/netutil: export netutil.LimitListenerConn Dec 22, 2023
@seankhliao seankhliao modified the milestones: Unreleased, Proposal Dec 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Proposal
Projects
Proposals
Status: Incoming
Milestone
Proposal
Development

No branches or pull requests
4 participants
@zmt @gopherbot @seankhliao @thanm