add option to set ResponseHeaderTimeout per client

this is necessary as in some cases we want a low timeout (when we
expect a quick response, e.g. with backup), but in others we may want
a very long timeout (long running query).

Now we have more granular control over timeouts so we can get things
to fail more predictably in tests.

ctl/backup.go

@@ -42,6 +42,9 @@ type BackupCommand struct { // nolint: maligned
 	// Amount of time after first failed request to continue retrying.
 	RetryPeriod time.Duration `json:"retry-period"`
 
+	// Response Header Timeout for HTTP Requests
+	HeaderTimeout time.Duration `json:"header-timeout"`
+
 	// Host:port on which to listen for pprof.
 	Pprof string `json:"pprof"`
 
@@ -59,10 +62,11 @@ type BackupCommand struct { // nolint: maligned
 // NewBackupCommand returns a new instance of BackupCommand.
 func NewBackupCommand(stdin io.Reader, stdout, stderr io.Writer) *BackupCommand {
 	return &BackupCommand{
-		CmdIO:       pilosa.NewCmdIO(stdin, stdout, stderr),
-		Concurrency: 1,
-		RetryPeriod: time.Minute,
-		Pprof:       "localhost:0",
+		CmdIO:         pilosa.NewCmdIO(stdin, stdout, stderr),
+		Concurrency:   1,
+		RetryPeriod:   time.Minute,
+		HeaderTimeout: time.Second * 3,
+		Pprof:         "localhost:0",
 	}
 }
 
@@ -89,7 +93,7 @@ func (cmd *BackupCommand) Run(ctx context.Context) (err error) {
 	}
 
 	// Create a client to the server.
-	client, err := commandClient(cmd, fb_http.WithClientRetryPeriod(cmd.RetryPeriod))
+	client, err := commandClient(cmd, fb_http.WithClientRetryPeriod(cmd.RetryPeriod), fb_http.ClientResponseHeaderTimeoutOption(cmd.HeaderTimeout))
 	if err != nil {
 		return fmt.Errorf("creating client: %w", err)
 	}
@@ -285,7 +289,9 @@ func (cmd *BackupCommand) backupShardNode(ctx context.Context, indexName string,
 	logger := cmd.Logger()
 	logger.Printf("backing up shard: index=%q id=%d", indexName, shard)
 
-	client := fb_http.NewInternalClientFromURI(&node.URI, fb_http.GetHTTPClient(cmd.tlsConfig), fb_http.WithClientRetryPeriod(cmd.RetryPeriod))
+	client := fb_http.NewInternalClientFromURI(&node.URI,
+		fb_http.GetHTTPClient(cmd.tlsConfig, fb_http.ClientResponseHeaderTimeoutOption(cmd.HeaderTimeout)),
+		fb_http.WithClientRetryPeriod(cmd.RetryPeriod))
 	rc, err := client.ShardReader(ctx, indexName, shard)
 	if err != nil {
 		return fmt.Errorf("fetching shard reader: %w", err)

ctl/common.go

@@ -2,11 +2,8 @@
 package ctl
 
 import (
-	"net"
 	"time"
 
-	gohttp "net/http"
-
 	"github.com/molecula/featurebase/v3/http"
 	"github.com/molecula/featurebase/v3/logger"
 	"github.com/molecula/featurebase/v3/server"
@@ -30,24 +27,49 @@ func SetTLSConfig(flags *pflag.FlagSet, prefix string, certificatePath *string,
 	flags.BoolVarP(enableClientVerification, prefix+"tls.enable-client-verification", "", false, "Enable TLS certificate client verification for incoming connections")
 }
 
-// default dial timeout is 30s for some reason which makes testing
-// failures/retries really awkward. I don't think we need it that
-// high, so I set it to 1s here... let's see what happens.
-func clientOptions(client *gohttp.Client, dialer *net.Dialer) *gohttp.Client {
-	dialer.Timeout = time.Second * 1
-	return client
-}
+// AnyClientOption can be either http.InternalClientOption or
+// http.ClientOption. The internal options are specific to the
+// featurebase client, whereas the client options are applied to the
+// Go HTTP client that gets used under the hood.
+type AnyClientOption interface{}
 
 // commandClient returns a pilosa.InternalHTTPClient for the command
-func commandClient(cmd CommandWithTLSSupport, opts ...http.InternalClientOption) (*http.InternalClient, error) {
+func commandClient(cmd CommandWithTLSSupport, opts ...AnyClientOption) (*http.InternalClient, error) {
+	internalopts, clientopts, err := separateOptions(opts...)
+	if err != nil {
+		return nil, errors.Wrap(err, "separating client options")
+	}
+
+	// we default dial timeout to 3s in commandClient, but prepend it
+	// to the option list so other options can override it.
+	clientopts = append([]http.ClientOption{http.ClientDialTimeoutOption(time.Second * 3)}, clientopts...)
 	tls := cmd.TLSConfiguration()
 	tlsConfig, err := server.GetTLSConfig(&tls, cmd.Logger())
 	if err != nil {
 		return nil, errors.Wrap(err, "getting tls config")
 	}
-	client, err := http.NewInternalClient(cmd.TLSHost(), http.GetHTTPClient(tlsConfig, clientOptions), opts...)
+	client, err := http.NewInternalClient(cmd.TLSHost(), http.GetHTTPClient(tlsConfig, clientopts...), internalopts...)
 	if err != nil {
 		return nil, errors.Wrap(err, "getting internal client")
 	}
 	return client, err
 }
+
+// separateOptions splits the list of AnyClientOption into the two
+// possible types.
+func separateOptions(opts ...AnyClientOption) ([]http.InternalClientOption, []http.ClientOption, error) {
+	internalopts := []http.InternalClientOption{}
+	clientopts := []http.ClientOption{}
+	for _, opt := range opts {
+		if iopt, ok := opt.(http.InternalClientOption); ok {
+			internalopts = append(internalopts, iopt)
+			continue
+		}
+		if copt, ok := opt.(http.ClientOption); ok {
+			clientopts = append(clientopts, copt)
+			continue
+		}
+		return nil, nil, errors.Errorf("opt: %+v of type %[1]T must be an InternalClientOption or a ClientOption", opt)
+	}
+	return internalopts, clientopts, nil
+}

http/handler.go

@@ -2864,9 +2864,23 @@ func (s queryValidationSpec) validate(query url.Values) error {
 
 type ClientOption func(client *http.Client, dialer *net.Dialer) *http.Client
 
+func ClientResponseHeaderTimeoutOption(dur time.Duration) ClientOption {
+	return func(client *http.Client, dialer *net.Dialer) *http.Client {
+		client.Transport.(*http.Transport).ResponseHeaderTimeout = dur
+		return client
+	}
+}
+
+func ClientDialTimeoutOption(dur time.Duration) ClientOption {
+	return func(client *http.Client, dialer *net.Dialer) *http.Client {
+		dialer.Timeout = dur
+		return client
+	}
+}
+
 func GetHTTPClient(t *tls.Config, opts ...ClientOption) *http.Client {
 	dialer := &net.Dialer{
-		Timeout:   5 * time.Second,
+		Timeout:   30 * time.Second,
 		KeepAlive: 15 * time.Second,
 		DualStack: true,
 	}
@@ -2878,7 +2892,6 @@ func GetHTTPClient(t *tls.Config, opts ...ClientOption) *http.Client {
 		IdleConnTimeout:       20 * time.Second,
 		TLSHandshakeTimeout:   10 * time.Second,
 		ExpectContinueTimeout: 1 * time.Second,
-		ResponseHeaderTimeout: 4 * time.Second,
 	}
 	if t != nil {
 		transport.TLSClientConfig = t

internal/clustertests/cluster_test.go

@@ -236,6 +236,16 @@ func TestClusterStuff(t *testing.T) {
 			t.Fatalf("restore failed: %v", err)
 		}
 
+		fmt.Println("pausing all featurebasen")
+		if err = sendCmd("docker", "pause", container(t, "pilosa1")); err != nil {
+			t.Fatalf("sending pause command: %v", err)
+		}
+		if err = sendCmd("docker", "pause", container(t, "pilosa2")); err != nil {
+			t.Fatalf("sending pause command: %v", err)
+		}
+		if err = sendCmd("docker", "pause", container(t, "pilosa3")); err != nil {
+			t.Fatalf("sending pause command: %v", err)
+		}
 		// now do backup with all nodes down and too short a timeout
 		// so it fails. Has be to be all 3 because the cluster has
 		// replicas=3 and the backup command will retry on replicas.
@@ -250,21 +260,10 @@ func TestClusterStuff(t *testing.T) {
 				t.Fatalf("sending second backup command: %v", err)
 			}
 		}
-		time.Sleep(time.Millisecond * 10) // want the backup to get started, then fail
-		fmt.Println("pausing all featurebasen")
-		if err = sendCmd("docker", "pause", container(t, "pilosa1")); err != nil {
-			t.Fatalf("sending pause command: %v", err)
-		}
-		if err = sendCmd("docker", "pause", container(t, "pilosa2")); err != nil {
-			t.Fatalf("sending pause command: %v", err)
-		}
-		if err = sendCmd("docker", "pause", container(t, "pilosa3")); err != nil {
-			t.Fatalf("sending pause command: %v", err)
-		}
 
-		fmt.Println("sleeping long")
-		time.Sleep(time.Second * 10)
-		fmt.Println("restarting")
+		t.Logf("sleeping 8s")
+		time.Sleep(time.Second * 8)
+		t.Logf("restarting FB nodes")
 
 		if err = sendCmd("docker", "unpause", container(t, "pilosa1")); err != nil {
 			t.Fatalf("sending unpause command: %v", err)