From a5ea2b9ba0f998aa27a280838bd81817a427ba34 Mon Sep 17 00:00:00 2001 From: Euan Kemp Date: Wed, 24 Jul 2024 08:32:42 +0000 Subject: [PATCH 1/3] Close all old file descriptors on update This is a breaking change, but it actually matches what the godoc says we do, so that seems good! This is breaking enough I'm bumping up to v3 for this one, but I think this is the right semantics most of the time. --- fds.go | 60 ++++++++++++++++++++++++++++++++++++++--------------- upgrader.go | 6 ++++++ 2 files changed, 49 insertions(+), 17 deletions(-) diff --git a/fds.go b/fds.go index 0082999..5df3313 100644 --- a/fds.go +++ b/fds.go @@ -2,6 +2,7 @@ package tableroll import ( "context" + "errors" "fmt" "net" "os" @@ -9,7 +10,6 @@ import ( "syscall" "github.com/inconshreveable/log15" - "github.com/pkg/errors" "golang.org/x/sys/unix" ) @@ -88,6 +88,11 @@ type fd struct { // The underlying file object file *file + // used is set to true the first time this file is used. + // It's here to close inherited FDs which are unused in + // the new process automatically. + used bool + Kind fdKind `json:"kind"` // ID is the id of this file, stored just for pretty-printing ID string `json:"id"` @@ -170,12 +175,13 @@ func (f *Fds) Listen(ctx context.Context, id string, cfg *net.ListenConfig, netw cfg = &net.ListenConfig{} } - ln, err := f.listenerLocked(id) + ln, fd, err := f.listenerLocked(id) if err != nil { return nil, err } if ln != nil { f.l.Debug("found existing listener in store", "listenerId", id, "network", network, "addr", addr) + fd.used = true return ln, nil } @@ -185,13 +191,13 @@ func (f *Fds) Listen(ctx context.Context, id string, cfg *net.ListenConfig, netw ln, err = cfg.Listen(ctx, network, addr) if err != nil { - return nil, errors.Wrap(err, "can't create new listener") + return nil, fmt.Errorf("can't create new listener: %w", err) } fdLn, ok := ln.(Listener) if !ok { ln.Close() - return nil, errors.Errorf("%T doesn't implement tableroll.Listener", ln) + return nil, fmt.Errorf("%T doesn't implement tableroll.Listener", ln) } err = f.addListenerLocked(id, network, addr, fdLn) @@ -214,11 +220,12 @@ func (f *Fds) ListenWith(id, network, addr string, listenerFunc func(network, ad f.mu.Lock() defer f.mu.Unlock() - ln, err := f.listenerLocked(id) + ln, fd, err := f.listenerLocked(id) if err != nil { return nil, err } if ln != nil { + fd.used = true return ln, nil } if f.locked { @@ -231,7 +238,7 @@ func (f *Fds) ListenWith(id, network, addr string, listenerFunc func(network, ad } if _, ok := ln.(Listener); !ok { ln.Close() - return nil, errors.Errorf("%T doesn't implement tableroll.Listener", ln) + return nil, fmt.Errorf("%T doesn't implement tableroll.Listener", ln) } if err := f.addListenerLocked(id, network, addr, ln.(Listener)); err != nil { ln.Close() @@ -248,20 +255,21 @@ func (f *Fds) Listener(id string) (net.Listener, error) { f.mu.Lock() defer f.mu.Unlock() - return f.listenerLocked(id) + ln, _, err := f.listenerLocked(id) + return ln, err } -func (f *Fds) listenerLocked(id string) (net.Listener, error) { +func (f *Fds) listenerLocked(id string) (net.Listener, *fd, error) { file, ok := f.fds[id] if !ok || file.file == nil { - return nil, nil + return nil, nil, nil } ln, err := net.FileListener(file.file.File) if err != nil { - return nil, errors.Wrapf(err, "can't inherit listener %s", file.file) + return nil, nil, fmt.Errorf("can't inherit listener %s: %w", file.file, err) } - return ln, nil + return ln, file, nil } type unlinkOnCloser interface { @@ -302,7 +310,7 @@ func (f *Fds) DialWith(id, network, address string, dialFn func(network, address fdConn, ok := newConn.(Conn) if !ok { newConn.Close() - return nil, errors.Errorf("%T doesn't implement tableroll.Conn", newConn) + return nil, fmt.Errorf("%T doesn't implement tableroll.Conn", newConn) } if err := f.addConnLocked(id, fdKindConn, network, address, fdConn); err != nil { @@ -331,13 +339,14 @@ func (f *Fds) connLocked(id string) (net.Conn, error) { conn, err := net.FileConn(file.file.File) if err != nil { - return nil, errors.Wrapf(err, "can't inherit connection %s", file.file) + return nil, fmt.Errorf("can't inherit connection %s: %w", file.file, err) } return conn, nil } func (f *Fds) addConnLocked(id string, kind fdKind, network, addr string, conn syscall.Conn) error { fdObj := &fd{ + used: true, Kind: kind, ID: id, Network: network, @@ -345,7 +354,7 @@ func (f *Fds) addConnLocked(id string, kind fdKind, network, addr string, conn s } file, err := dupConn(conn, fdObj.String()) if err != nil { - return errors.Wrapf(err, "can't dup listener %s %s", network, addr) + return fmt.Errorf("can't dup listener %s %s: %w", network, addr, err) } fdObj.file = file f.fds[id] = fdObj @@ -381,6 +390,7 @@ func (f *Fds) OpenFileWith(id string, name string, openFunc func(name string) (* } newFd := &fd{ + used: true, ID: id, Kind: fdKindFile, file: dup, @@ -413,7 +423,7 @@ func (f *Fds) Remove(id string) error { item, ok := f.fds[id] if !ok { - return errors.Errorf("no element in map with id %v", id) + return fmt.Errorf("no element in map with id %v", id) } delete(f.fds, id) if item.file != nil { @@ -449,6 +459,22 @@ func (f *Fds) copy() map[string]*fd { return files } +// closeUnused closes unused FDs. It should be called +// while Fds is locked. +func (f *Fds) closeUnused() error { + var errs []error + for name, fd := range f.fds { + if !fd.used { + err := fd.file.Close() + if err != nil { + errs = append(errs, fmt.Errorf("error closing %v: %w", name, err)) + } + delete(f.fds, name) + } + } + return errors.Join(errs...) +} + func dupConn(conn syscall.Conn, name string) (*file, error) { // Use SyscallConn instead of File to avoid making the original // fd non-blocking. @@ -463,7 +489,7 @@ func dupConn(conn syscall.Conn, name string) (*file, error) { dup, duperr = dupFd(fd, name) }) if err != nil { - return nil, errors.Wrap(err, "can't access fd") + return nil, fmt.Errorf("can't access fd: %w", err) } return dup, duperr } @@ -471,7 +497,7 @@ func dupConn(conn syscall.Conn, name string) (*file, error) { func dupFd(fd uintptr, name string) (*file, error) { dupfd, _, errno := unix.Syscall(unix.SYS_FCNTL, fd, unix.F_DUPFD_CLOEXEC, 0) if errno != 0 { - return nil, errors.Wrap(errno, "can't dup fd using fcntl") + return nil, fmt.Errorf("can't dup fd using fcntl: %v", errno) } return newFile(dupfd, name), nil diff --git a/upgrader.go b/upgrader.go index 117e440..b526357 100644 --- a/upgrader.go +++ b/upgrader.go @@ -238,6 +238,12 @@ func (u *Upgrader) Ready() error { if err := u.state.transitionTo(upgraderStateOwner); err != nil { return err } + + // Now cleanup all old FDs while holding the lock + u.Fds.lockMutations(errors.New("closing old listeners")) + defer u.Fds.unlockMutations() + _ = u.Fds.closeUnused() + return nil } From d4ae97468b3e8b0e8688d64d77acecef83d1bd11 Mon Sep 17 00:00:00 2001 From: Euan Kemp Date: Thu, 25 Jul 2024 08:51:37 +0000 Subject: [PATCH 2/3] Bump to v3 The previous change is a breaking-ish change, or at least breaking enough that I think it warrants a v3 --- README.md | 2 +- go.mod | 2 +- sibling.go | 2 +- transfer_owner.go | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/README.md b/README.md index c1aae49..6d36380 100644 --- a/README.md +++ b/README.md @@ -49,7 +49,7 @@ import ( "time" "github.com/inconshreveable/log15" - "github.com/ngrok-oss/tableroll/v2" + "github.com/ngrok-oss/tableroll/v3" ) func main() { diff --git a/go.mod b/go.mod index 3a392e4..fdaefb0 100644 --- a/go.mod +++ b/go.mod @@ -1,4 +1,4 @@ -module github.com/ngrok-oss/tableroll/v2 +module github.com/ngrok-oss/tableroll/v3 require ( github.com/euank/filelock v0.0.0-20200318073246-6ea232a62104 diff --git a/sibling.go b/sibling.go index 920bf3e..5591017 100644 --- a/sibling.go +++ b/sibling.go @@ -6,7 +6,7 @@ import ( "time" "github.com/inconshreveable/log15" - "github.com/ngrok-oss/tableroll/v2/internal/proto" + "github.com/ngrok-oss/tableroll/v3/internal/proto" "github.com/pkg/errors" ) diff --git a/transfer_owner.go b/transfer_owner.go index 5590fd2..3f40c77 100644 --- a/transfer_owner.go +++ b/transfer_owner.go @@ -7,7 +7,7 @@ import ( "sync" "github.com/inconshreveable/log15" - "github.com/ngrok-oss/tableroll/v2/internal/proto" + "github.com/ngrok-oss/tableroll/v3/internal/proto" "github.com/pkg/errors" ) From 7b5a6dc855527f4cb05f47b719c19a3d46f1d7c4 Mon Sep 17 00:00:00 2001 From: Euan Kemp Date: Wed, 21 Aug 2024 09:05:26 +0000 Subject: [PATCH 3/3] Minor listener-used-marking cleanup from PR review --- fds.go | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/fds.go b/fds.go index 5df3313..79c5db6 100644 --- a/fds.go +++ b/fds.go @@ -175,13 +175,12 @@ func (f *Fds) Listen(ctx context.Context, id string, cfg *net.ListenConfig, netw cfg = &net.ListenConfig{} } - ln, fd, err := f.listenerLocked(id) + ln, err := f.listenerLocked(id) if err != nil { return nil, err } if ln != nil { f.l.Debug("found existing listener in store", "listenerId", id, "network", network, "addr", addr) - fd.used = true return ln, nil } @@ -220,12 +219,11 @@ func (f *Fds) ListenWith(id, network, addr string, listenerFunc func(network, ad f.mu.Lock() defer f.mu.Unlock() - ln, fd, err := f.listenerLocked(id) + ln, err := f.listenerLocked(id) if err != nil { return nil, err } if ln != nil { - fd.used = true return ln, nil } if f.locked { @@ -255,21 +253,23 @@ func (f *Fds) Listener(id string) (net.Listener, error) { f.mu.Lock() defer f.mu.Unlock() - ln, _, err := f.listenerLocked(id) + ln, err := f.listenerLocked(id) return ln, err } -func (f *Fds) listenerLocked(id string) (net.Listener, *fd, error) { +func (f *Fds) listenerLocked(id string) (net.Listener, error) { file, ok := f.fds[id] if !ok || file.file == nil { - return nil, nil, nil + return nil, nil } ln, err := net.FileListener(file.file.File) if err != nil { - return nil, nil, fmt.Errorf("can't inherit listener %s: %w", file.file, err) + return nil, fmt.Errorf("can't inherit listener %s: %w", file.file, err) } - return ln, file, nil + // now that we've converted this into a go listener, assume it's used + file.used = true + return ln, nil } type unlinkOnCloser interface {