add a configurable timeout to http handler closing

refactor handler Close func to use errgroup to be a bit less messy.

refactor pilosa.Server closing to actually return an underlying error if one occurs

add option to pilosa/test.Cluster and pilosa/server.Command to control close
timeout. currently is only used by the http handler, but conceivably could be
passed as a parameter to other subsystems of pilosa/server.Command

http/handler.go

@@ -55,6 +55,8 @@ type Handler struct {
 
 	ln net.Listener
 
+	closeTimeout time.Duration
+
 	server *http.Server
 }
 
@@ -109,10 +111,20 @@ func OptHandlerListener(ln net.Listener) handlerOption {
 	}
 }
 
+// OptHandlerCloseTimeout controls how long we'll wait for the http Server to
+// shutdown cleanly before forcibly destroying it. Default is 30 seconds.
+func OptHandlerCloseTimeout(d time.Duration) handlerOption {
+	return func(h *Handler) error {
+		h.closeTimeout = d
+		return nil
+	}
+}
+
 // NewHandler returns a new instance of Handler with a default logger.
 func NewHandler(opts ...handlerOption) (*Handler, error) {
 	handler := &Handler{
-		logger: pilosa.NopLogger,
+		logger:       pilosa.NopLogger,
+		closeTimeout: time.Second * 30,
 	}
 	handler.Handler = newRouter(handler)
 	handler.populateValidators()
@@ -146,10 +158,16 @@ func (h *Handler) Serve() error {
 	return nil
 }
 
+// Close tries to cleanly shutdown the HTTP server, and failing that, after a
+// timeout, calls Server.Close.
 func (h *Handler) Close() error {
-	// TODO: timeout?
-	err := h.server.Shutdown(context.Background())
-	return errors.Wrap(err, "shutdown http server")
+	deadlineCtx, cancelFunc := context.WithDeadline(context.Background(), time.Now().Add(h.closeTimeout))
+	defer cancelFunc()
+	err := h.server.Shutdown(deadlineCtx)
+	if err != nil {
+		err = h.server.Close()
+	}
+	return errors.Wrap(err, "shutdown/close http server")
 }
 
 func (h *Handler) populateValidators() {

server.go

@@ -369,17 +369,28 @@ func (s *Server) Close() error {
 	close(s.closing)
 	s.wg.Wait()
 
+	var errh error
+	var errt error
+	var errc error
 	if s.cluster != nil {
-		s.cluster.close()
+		errc = s.cluster.close()
 	}
 	if s.holder != nil {
-		s.holder.Close()
+		errh = s.holder.Close()
 	}
 	if s.translateFile != nil {
-		s.translateFile.Close()
+		errt = s.translateFile.Close()
 	}
-
-	return nil
+	// prefer to return handler error over translateFile error over cluster
+	// error. This order is somewhat arbitrary. It would be better if we had
+	// some way to combine all the errors, but probably not important enough to
+	// warrant the extra complexity.
+	if errh != nil {
+		return errors.Wrap(errh, "closing handler")
+	} else if errt != nil {
+		return errors.Wrap(errt, "closing translatFile")
+	}
+	return errors.Wrap(errc, "closing cluster")
 }
 
 // loadNodeID gets NodeID from disk, or creates a new value.

server/server.go

@@ -20,7 +20,7 @@
 package server
 
 import (
-	"fmt"
+	"crypto/tls"
 	"io"
 	"log"
 	"math/rand"
@@ -31,7 +31,7 @@ import (
 	"syscall"
 	"time"
 
-	"crypto/tls"
+	"golang.org/x/sync/errgroup"
 
 	"github.com/pilosa/pilosa"
 	"github.com/pilosa/pilosa/boltdb"
@@ -76,9 +76,10 @@ type Command struct {
 	logOutput io.Writer
 	logger    loggerLogger
 
-	Handler pilosa.Handler
-	API     *pilosa.API
-	ln      net.Listener
+	Handler      pilosa.Handler
+	API          *pilosa.API
+	ln           net.Listener
+	closeTimeout time.Duration
 
 	serverOptions []pilosa.ServerOption
 }
@@ -92,6 +93,13 @@ func OptCommandServerOptions(opts ...pilosa.ServerOption) CommandOption {
 	}
 }
 
+func OptCommandCloseTimeout(d time.Duration) CommandOption {
+	return func(c *Command) error {
+		c.closeTimeout = d
+		return nil
+	}
+}
+
 // NewCommand returns a new instance of Main.
 func NewCommand(stdin io.Reader, stdout, stderr io.Writer, opts ...CommandOption) *Command {
 	c := &Command{
@@ -295,6 +303,7 @@ func (m *Command) SetupServer() error {
 		http.OptHandlerAPI(m.API),
 		http.OptHandlerLogger(m.logger),
 		http.OptHandlerListener(m.ln),
+		http.OptHandlerCloseTimeout(m.closeTimeout),
 	)
 	return errors.Wrap(err, "new handler")
 
@@ -341,21 +350,18 @@ func (m *Command) GossipTransport() *gossip.Transport {
 
 // Close shuts down the server.
 func (m *Command) Close() error {
-	var logErr error
-	handlerErr := m.Handler.Close()
-	serveErr := m.Server.Close()
-	var gossipErr error
+	defer close(m.done)
+	eg := errgroup.Group{}
+	eg.Go(m.Handler.Close)
+	eg.Go(m.Server.Close)
 	if m.gossipMemberSet != nil {
-		gossipErr = m.gossipMemberSet.Close()
+		eg.Go(m.gossipMemberSet.Close)
 	}
 	if closer, ok := m.logOutput.(io.Closer); ok {
-		logErr = closer.Close()
+		eg.Go(closer.Close)
 	}
-	close(m.done)
-	if serveErr != nil || logErr != nil || handlerErr != nil || gossipErr != nil {
-		return fmt.Errorf("closing server: '%v', closing logs: '%v', closing handler: '%v', closing gossip: '%v'", serveErr, logErr, handlerErr, gossipErr)
-	}
-	return nil
+	err := eg.Wait()
+	return errors.Wrap(err, "closing everything")
 }
 
 // newStatsClient creates a stats client from the config

test/pilosa.go

@@ -23,6 +23,7 @@ import (
 	"os"
 	"strings"
 	"testing"
+	"time"
 
 	"github.com/pilosa/pilosa/http"
 	"github.com/pilosa/pilosa/server"
@@ -55,6 +56,11 @@ func newCommand(opts ...server.CommandOption) *Command {
 		panic(err)
 	}
 
+	// set aggressive close timeout by default to avoid hanging tests. This was
+	// a probably with PDK tests which used go-pilosa as well. We put it at the
+	// beginning of the option slice so that it can be overridden by an
+	// user-passed options.
+	opts = append([]server.CommandOption{server.OptCommandCloseTimeout(time.Millisecond * 2)}, opts...)
 	m := &Command{Command: server.NewCommand(os.Stdin, os.Stdout, os.Stderr, opts...), commandOptions: opts}
 	m.Config.DataDir = path
 	m.Config.Bind = "http://localhost:0"