Skip to content

Documented behavior for custom http.Transport wrt MaxIdleConnsPerHost field #93

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.

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

Merged
merged 3 commits into from
Feb 9, 2018
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 39 additions & 3 deletions http/connection.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,10 +28,12 @@ import (
"encoding/json"
"fmt"
"io/ioutil"
"net"
"net/http"
"net/http/httptrace"
"net/url"
"strings"
"time"

driver "github.com/arangodb/go-driver"
"github.com/arangodb/go-driver/cluster"
Expand All @@ -40,6 +42,8 @@ import (
)

const (
DefaultMaxIdleConnsPerHost = 64

keyRawResponse driver.ContextKey = "arangodb-rawResponse"
keyResponse driver.ContextKey = "arangodb-response"
)
Expand All @@ -56,6 +60,11 @@ type ConnectionConfig struct {
// If Transport is not of type `*http.Transport`, the `TLSConfig` property is not used.
// Otherwise a `TLSConfig` property other than `nil` will overwrite the `TLSClientConfig`
// property of `Transport`.
//
// When using a custom `http.Transport`, make sure to set the `MaxIdleConnsPerHost` field at least as
// high as the maximum number of concurrent requests you will make to your database.
// A lower number will cause the golang runtime to create additional connections and close them
// directly after use, resulting in a large number of connections in `TIME_WAIT` state.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should add a comment here that we set the default to at least 64 and that one should lower it if needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a line in master

Transport http.RoundTripper
// FailOnRedirect; if set, redirect will not be followed, instead the status code is returned as error
FailOnRedirect bool
Expand Down Expand Up @@ -91,11 +100,38 @@ func newHTTPConnection(endpoint string, config ConnectionConfig) (driver.Connect
if config.Transport != nil {
httpTransport, _ = config.Transport.(*http.Transport)
} else {
httpTransport = &http.Transport{}
httpTransport = &http.Transport{
// Copy default values from http.DefaultTransport
Proxy: http.ProxyFromEnvironment,
DialContext: (&net.Dialer{
Timeout: 30 * time.Second,
KeepAlive: 30 * time.Second,
DualStack: true,
}).DialContext,
MaxIdleConns: 100,
IdleConnTimeout: 90 * time.Second,
TLSHandshakeTimeout: 10 * time.Second,
ExpectContinueTimeout: 1 * time.Second,
}
config.Transport = httpTransport
}
if config.TLSConfig != nil && httpTransport != nil {
httpTransport.TLSClientConfig = config.TLSConfig
if httpTransport != nil {
if httpTransport.MaxIdleConnsPerHost == 0 {
// Raise the default number of idle connections per host since in a database application
// it is very likely that you want more than 2 concurrent connections to a host.
// We raise it to avoid the extra concurrent connections being closed directly
// after use, resulting in a lot of connection in `TIME_WAIT` state.
httpTransport.MaxIdleConnsPerHost = DefaultMaxIdleConnsPerHost
}
defaultMaxIdleConns := 3 * DefaultMaxIdleConnsPerHost
if httpTransport.MaxIdleConns > 0 && httpTransport.MaxIdleConns < defaultMaxIdleConns {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems MaxIdleConnsPerHost is set to 0 here if nothing is set, is the same true for MaxIdleConns? If so, then we should write "== 0" here, otherwise we do not set out default if the user has not given anything.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Behavior for MaxIdleConns==0 is different. It means "unlimited"

// For a cluster scenario we assume the use of 3 coordinators (don't know the exact number here)
// and derive the maximum total number of idle connections from that.
httpTransport.MaxIdleConns = defaultMaxIdleConns
}
if config.TLSConfig != nil {
httpTransport.TLSClientConfig = config.TLSConfig
}
}
httpClient := &http.Client{
Transport: config.Transport,
Expand Down