From 08fa90e0f3dddcb88fbe43c95f861214cbe891bd Mon Sep 17 00:00:00 2001 From: Cody Soyland Date: Tue, 14 Apr 2020 16:11:34 -0500 Subject: [PATCH 1/6] Reduce duplication of setupLogger arch-specific code --- server/dup.go | 26 +++++++++++++++++++ server/dup_arm64.go | 24 ++++++++++++++++++ server/server.go | 24 ++++++++++++++++++ server/setup_logger.go | 49 ------------------------------------ server/setup_logger_arm64.go | 47 ---------------------------------- 5 files changed, 74 insertions(+), 96 deletions(-) create mode 100644 server/dup.go create mode 100644 server/dup_arm64.go delete mode 100644 server/setup_logger.go delete mode 100644 server/setup_logger_arm64.go diff --git a/server/dup.go b/server/dup.go new file mode 100644 index 000000000..63d871054 --- /dev/null +++ b/server/dup.go @@ -0,0 +1,26 @@ +// Copyright 2017 Pilosa Corp. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +// +build !arm64 + +package server + +import ( + "syscall" +) + +// dup is an alias for syscall.Dup2 on most platforms or syscall.Dup3 on ARM64 +func (m *Command) dup(oldfd int, newfd int) error { + return syscall.Dup2(oldfd, newfd) +} diff --git a/server/dup_arm64.go b/server/dup_arm64.go new file mode 100644 index 000000000..726890fec --- /dev/null +++ b/server/dup_arm64.go @@ -0,0 +1,24 @@ +// Copyright 2017 Pilosa Corp. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package server + +import ( + "syscall" +) + +// dup is an alias for syscall.Dup2 on most platforms or syscall.Dup3 on ARM +func (m *Command) dup(oldfd int, newfd int) error { + return syscall.Dup3(oldfd, newfd, 0) +} diff --git a/server/server.go b/server/server.go index 80fccf4f9..a65ba09be 100644 --- a/server/server.go +++ b/server/server.go @@ -414,6 +414,30 @@ func (m *Command) setupNetworking() error { return errors.Wrap(gossipMemberSet.Open(), "opening gossip memberset") } +// setupLogger sets up the logger based on the configuration. +func (m *Command) setupLogger() error { + if m.Config.LogPath == "" { + m.logOutput = m.Stderr + } else { + f, err := os.OpenFile(m.Config.LogPath, os.O_RDWR|os.O_CREATE|os.O_APPEND, 0600) + if err != nil { + return errors.Wrap(err, "opening file") + } + m.logOutput = f + err = m.dup(int(f.Fd()), int(os.Stderr.Fd())) + if err != nil { + return errors.Wrap(err, "syscall dup stderr to logfile") + } + } + + if m.Config.Verbose { + m.logger = logger.NewVerboseLogger(m.logOutput) + } else { + m.logger = logger.NewStandardLogger(m.logOutput) + } + return nil +} + // GossipTransport allows a caller to return the gossip transport created when // setting up the GossipMemberSet. This is useful if one needs to determine the // allocated ephemeral port programmatically. (usually used in tests) diff --git a/server/setup_logger.go b/server/setup_logger.go deleted file mode 100644 index 3dd0ff849..000000000 --- a/server/setup_logger.go +++ /dev/null @@ -1,49 +0,0 @@ -// Copyright 2017 Pilosa Corp. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -// +build !arm64 - -package server - -import ( - "os" - "syscall" - - "github.com/pilosa/pilosa/v2/logger" - "github.com/pkg/errors" -) - -// setupLogger sets up the logger based on the configuration. -func (m *Command) setupLogger() error { - if m.Config.LogPath == "" { - m.logOutput = m.Stderr - } else { - f, err := os.OpenFile(m.Config.LogPath, os.O_RDWR|os.O_CREATE|os.O_APPEND, 0600) - if err != nil { - return errors.Wrap(err, "opening file") - } - m.logOutput = f - err = syscall.Dup2(int(f.Fd()), int(os.Stderr.Fd())) - if err != nil { - return errors.Wrap(err, "dup2ing stderr onto logfile") - } - } - - if m.Config.Verbose { - m.logger = logger.NewVerboseLogger(m.logOutput) - } else { - m.logger = logger.NewStandardLogger(m.logOutput) - } - return nil -} diff --git a/server/setup_logger_arm64.go b/server/setup_logger_arm64.go deleted file mode 100644 index 4a0591582..000000000 --- a/server/setup_logger_arm64.go +++ /dev/null @@ -1,47 +0,0 @@ -// Copyright 2017 Pilosa Corp. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package server - -import ( - "os" - "syscall" - - "github.com/pilosa/pilosa/v2/logger" - "github.com/pkg/errors" -) - -// setupLogger sets up the logger based on the configuration. -func (m *Command) setupLogger() error { - if m.Config.LogPath == "" { - m.logOutput = m.Stderr - } else { - f, err := os.OpenFile(m.Config.LogPath, os.O_RDWR|os.O_CREATE|os.O_APPEND, 0600) - if err != nil { - return errors.Wrap(err, "opening file") - } - m.logOutput = f - err = syscall.Dup3(int(f.Fd()), int(os.Stderr.Fd()), 0) - if err != nil { - return errors.Wrap(err, "dup2ing stderr onto logfile") - } - } - - if m.Config.Verbose { - m.logger = logger.NewVerboseLogger(m.logOutput) - } else { - m.logger = logger.NewStandardLogger(m.logOutput) - } - return nil -} From a66ee26a6b487065db66c487027ce4f816bc5599 Mon Sep 17 00:00:00 2001 From: Cody Soyland Date: Tue, 14 Apr 2020 16:35:39 -0500 Subject: [PATCH 2/6] Reopen log file on SIGHUP --- go.mod | 3 +++ go.sum | 2 ++ server/server.go | 22 +++++++++++++++++----- 3 files changed, 22 insertions(+), 5 deletions(-) diff --git a/go.mod b/go.mod index 9d9ecd366..18e398490 100644 --- a/go.mod +++ b/go.mod @@ -2,12 +2,15 @@ module github.com/pilosa/pilosa/v2 replace github.com/hashicorp/memberlist => github.com/pilosa/memberlist v0.1.4-0.20190415211605-f6512523c021 +replace github.com/client9/reopen => github.com/codysoyland/reopen v1.0.1-0.20200414204206-42cbe848be3b + require ( github.com/CAFxX/gcnotifier v0.0.0-20190112062741-224a280d589d github.com/DataDog/datadog-go v0.0.0-20180822151419-281ae9f2d895 github.com/StackExchange/wmi v0.0.0-20190523213315-cbe66965904d // indirect github.com/boltdb/bolt v1.3.1 github.com/cespare/xxhash v1.1.0 + github.com/client9/reopen v0.0.0-00010101000000-000000000000 github.com/codahale/hdrhistogram v0.0.0-20161010025455-3a0bb77429bd // indirect github.com/davecgh/go-spew v1.1.1 github.com/go-ole/go-ole v1.2.4 // indirect diff --git a/go.sum b/go.sum index 9da5feace..109caa573 100644 --- a/go.sum +++ b/go.sum @@ -26,6 +26,8 @@ github.com/client9/misspell v0.3.4/go.mod h1:qj6jICC3Q7zFZvVWo7KLAzC3yx5G7kyvSDk github.com/cncf/udpa/go v0.0.0-20191209042840-269d4d468f6f/go.mod h1:M8M6+tZqaGXZJjfX53e64911xZQV5JYwmTeXPW+k8Sc= github.com/codahale/hdrhistogram v0.0.0-20161010025455-3a0bb77429bd h1:qMd81Ts1T2OTKmB4acZcyKaMtRnY5Y44NuXGX2GFJ1w= github.com/codahale/hdrhistogram v0.0.0-20161010025455-3a0bb77429bd/go.mod h1:sE/e/2PUdi/liOCUjSTXgM1o87ZssimdTWN964YiIeI= +github.com/codysoyland/reopen v1.0.1-0.20200414204206-42cbe848be3b h1:CP/etmJf4LXC6I1MJQt+I7oh8geIkWllD/NT0T23Y0c= +github.com/codysoyland/reopen v1.0.1-0.20200414204206-42cbe848be3b/go.mod h1:8fFEqM7bujfAJw/3T2Z/K8FMh+5vEsZ/8eDbNAVGFH8= github.com/coreos/etcd v3.3.10+incompatible/go.mod h1:uF7uidLiAD3TWHmW31ZFd/JWoc32PjwdhPthX9715RE= github.com/coreos/go-etcd v2.0.0+incompatible/go.mod h1:Jez6KQU2B/sWsbdaef3ED8NzMklzPG4d5KIOhIy30Tk= github.com/coreos/go-semver v0.2.0/go.mod h1:nnelYz7RCh+5ahJtPPxZlU+153eP4D4r3EedlOD2RNk= diff --git a/server/server.go b/server/server.go index a65ba09be..70d30ab2c 100644 --- a/server/server.go +++ b/server/server.go @@ -36,6 +36,7 @@ import ( "golang.org/x/sync/errgroup" + "github.com/client9/reopen" "github.com/pilosa/pilosa/v2" "github.com/pilosa/pilosa/v2/boltdb" "github.com/pilosa/pilosa/v2/encoding/proto" @@ -419,15 +420,26 @@ func (m *Command) setupLogger() error { if m.Config.LogPath == "" { m.logOutput = m.Stderr } else { - f, err := os.OpenFile(m.Config.LogPath, os.O_RDWR|os.O_CREATE|os.O_APPEND, 0600) + f, err := reopen.NewFileWriter(m.Config.LogPath) if err != nil { return errors.Wrap(err, "opening file") } + sighup := make(chan os.Signal, 1) + signal.Notify(sighup, syscall.SIGHUP) + go func() { + for { + // duplicate stderr onto log file + err = m.dup(int(f.Fd()), int(os.Stderr.Fd())) + if err != nil { + io.WriteString(f, "syscall dup error: "+err.Error()) + } + + // reopen log file on SIGHUP + <-sighup + f.Reopen() + } + }() m.logOutput = f - err = m.dup(int(f.Fd()), int(os.Stderr.Fd())) - if err != nil { - return errors.Wrap(err, "syscall dup stderr to logfile") - } } if m.Config.Verbose { From 2155d6cab8398984a121df1f621655af69af57a9 Mon Sep 17 00:00:00 2001 From: Cody Soyland Date: Tue, 14 Apr 2020 16:57:36 -0500 Subject: [PATCH 3/6] Move some things around, fix linter warnings. --- server/server.go | 27 ++++++++++++++++----------- 1 file changed, 16 insertions(+), 11 deletions(-) diff --git a/server/server.go b/server/server.go index 70d30ab2c..4f55b118f 100644 --- a/server/server.go +++ b/server/server.go @@ -416,14 +416,23 @@ func (m *Command) setupNetworking() error { } // setupLogger sets up the logger based on the configuration. -func (m *Command) setupLogger() error { +func (m *Command) setupLogger() (err error) { + var f *reopen.FileWriter if m.Config.LogPath == "" { m.logOutput = m.Stderr } else { - f, err := reopen.NewFileWriter(m.Config.LogPath) + f, err = reopen.NewFileWriter(m.Config.LogPath) if err != nil { return errors.Wrap(err, "opening file") } + m.logOutput = f + } + if m.Config.Verbose { + m.logger = logger.NewVerboseLogger(m.logOutput) + } else { + m.logger = logger.NewStandardLogger(m.logOutput) + } + if m.Config.LogPath != "" { sighup := make(chan os.Signal, 1) signal.Notify(sighup, syscall.SIGHUP) go func() { @@ -431,21 +440,17 @@ func (m *Command) setupLogger() error { // duplicate stderr onto log file err = m.dup(int(f.Fd()), int(os.Stderr.Fd())) if err != nil { - io.WriteString(f, "syscall dup error: "+err.Error()) + m.logger.Printf("syscall dup: %s\n", err.Error()) } // reopen log file on SIGHUP <-sighup - f.Reopen() + err = f.Reopen() + if err != nil { + m.logger.Printf("reopen: %s\n", err.Error()) + } } }() - m.logOutput = f - } - - if m.Config.Verbose { - m.logger = logger.NewVerboseLogger(m.logOutput) - } else { - m.logger = logger.NewStandardLogger(m.logOutput) } return nil } From a00e93f699cfc88064ea9d28d7798a9ed78da4bb Mon Sep 17 00:00:00 2001 From: Cody Soyland Date: Wed, 15 Apr 2020 15:45:57 -0500 Subject: [PATCH 4/6] Add TODO about fork --- go.mod | 1 + 1 file changed, 1 insertion(+) diff --git a/go.mod b/go.mod index 18e398490..ccdca5ab4 100644 --- a/go.mod +++ b/go.mod @@ -2,6 +2,7 @@ module github.com/pilosa/pilosa/v2 replace github.com/hashicorp/memberlist => github.com/pilosa/memberlist v0.1.4-0.20190415211605-f6512523c021 +// TODO: Remove the following line if this is merged: https://github.com/client9/reopen/pull/9 replace github.com/client9/reopen => github.com/codysoyland/reopen v1.0.1-0.20200414204206-42cbe848be3b require ( From e4cd1b887105f0efadf9d122b01bfeaa681fed9b Mon Sep 17 00:00:00 2001 From: Cody Soyland Date: Thu, 16 Apr 2020 17:04:54 -0500 Subject: [PATCH 5/6] Add -v flag to test-race --- Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Makefile b/Makefile index e0b639060..74e850ed7 100644 --- a/Makefile +++ b/Makefile @@ -46,7 +46,7 @@ test: # Run test suite with race flag test-race: case $$(go version) in *go1.14*) NOCHECKPTR="-gcflags=all=-d=checkptr=0";; *) NOCHECKPTR="";; esac ; \ - go test ./... -tags='$(BUILD_TAGS)' $(TESTFLAGS) -race $$NOCHECKPTR -timeout 30m + go test ./... -tags='$(BUILD_TAGS)' $(TESTFLAGS) -race $$NOCHECKPTR -timeout 30m -v bench: go test ./... -bench=. -run=NoneZ -timeout=127m $(TESTFLAGS) From 1058440cfe543f810c24a8bd5a660427972d1345 Mon Sep 17 00:00:00 2001 From: Cody Soyland Date: Thu, 16 Apr 2020 17:08:24 -0500 Subject: [PATCH 6/6] Do not reuse error object (data race) --- server/server.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/server/server.go b/server/server.go index 4f55b118f..dd4d9694b 100644 --- a/server/server.go +++ b/server/server.go @@ -416,8 +416,9 @@ func (m *Command) setupNetworking() error { } // setupLogger sets up the logger based on the configuration. -func (m *Command) setupLogger() (err error) { +func (m *Command) setupLogger() error { var f *reopen.FileWriter + var err error if m.Config.LogPath == "" { m.logOutput = m.Stderr } else { @@ -438,7 +439,7 @@ func (m *Command) setupLogger() (err error) { go func() { for { // duplicate stderr onto log file - err = m.dup(int(f.Fd()), int(os.Stderr.Fd())) + err := m.dup(int(f.Fd()), int(os.Stderr.Fd())) if err != nil { m.logger.Printf("syscall dup: %s\n", err.Error()) }