GODRIVER-3373 Prevent panic if http.DefaultTransport is not an *http.Transport

[jsphpl]

GODRIVER-3373

Summary

It is not safe to assume that http.DefaultTransport is always an *http.Transport. This patch prevents a panic that previously occured when the type assertion failed.

Background & Motivation

I am using https://github.com/motemen/go-loghttp to log http requests. This package replaces http.DefaultTransport with a *loghttp.Transport. It can therefore currently not be used together with mongo-go-driver.

Related Issues:

• Discussion in loghttp package: Panic when using global setting with kubernetes/go-client motemen/go-loghttp#3
• Fix for the same problem in google api client: fix(auth): handle non-Transport DefaultTransport googleapis/google-cloud-go#10162

[prestonvasquez]

@jsphpl thanks you so much for the PR! I don't see a reason for the Go Driver to declare it's own default client. Rather, we should rely just instantiate one as needed:

// NewHTTPClient will return the default HTTP client. The instantiaion will use
// the globally-defined DefaultTransport.
func NewHTTPClient() *http.Client {
	client := &http.Client{}
	if _, ok := http.DefaultTransport.(*http.Transport); !ok {
		client.Transport = http.DefaultTransport
	}

	return client
}

The solution you've proposed would not pass the following test, for example, as the globalized variable would have to be initialized before importing httputil (i.e. "go.mongodb.org/mongo-driver/v2/mongo"):

type nonDefaultTransport struct{}

var _ http.RoundTripper = &nonDefaultTransport{}

func (*nonDefaultTransport) RoundTrip(*http.Request) (*http.Response, error) { return nil, nil }

func TestDefaultHTTPClientTransport(t *testing.T) {
	t.Run("default", func(t *testing.T) {
		val := assert.ObjectsAreEqual(http.DefaultClient, NewHTTPClient())
		assert.True(t, val)
	})

	t.Run("non-default global transport", func(t *testing.T) {
		http.DefaultTransport = &nonDefaultTransport{}

		val := assert.ObjectsAreEqual(&nonDefaultTransport{}, NewHTTPClient().Transport)
		assert.True(t, val)
	})
}

More specifically, I'm guessing you are doing something like this:

_ "github.com/motemen/go-loghttp/global" // Just this line!
"go.mongodb.org/mongo-driver/v2/mongo"

The one gotcha with this solution is when disconnecting the client, here. In this case, I suggest comparing to http.Client:

if *c.httpClient == http.Client{} {
	defer httputil.CloseIdleHTTPConnections(c.httpClient)
}

LMK if this works for you. I've added this to the 1.17.2 patch release.

[jsphpl]

@prestonvasquez thank you for the feedback.

I don't see a reason for a global default client either. httputil.DefaultHTTPClient existed before and was referenced in a couple of places. The proposed patch is the least invasive approach I could come up with.

Yet I am not sure I am following you entirely. Are you proposing to replace httputil.DefaultHTTPClient by a func NewHTTPClient() and refactor those references to httputil.DefaultHTTPClient to instead use the factory function?

---

Edit:

If I am reading your func NewHTTPClient() correctly, it would alter the Transport of http.DefaultClient if the type of http.DefaultTransport is not *http.Transport. I don't think a third-party library should alter the Transport of the global http.DefaultClient.

Why do we even care about the client's transport in the first place? My naive expectation for any third-party library using an http.Client to communicate with some HTTP service would be:

1. Accept an http.Client via dependency injection
2. If no client is provided by the user via DI, fall back to using http.DefaultClient (but don't alter it)

[prestonvasquez]

@jsphpl

If I am reading your func NewHTTPClient() correctly, it would alter the Transport of http.DefaultClient if the type of http.DefaultTransport is not *http.Transport. I don't think a third-party library should alter the Transport of the global http.DefaultClient.

Good catch, updated.

My naive expectation for any third-party library using an http.Client to communicate with some HTTP service would be:

1. Accept an http.Client via dependency injection
2. If no client is provided by the user via DI, fall back to using http.DefaultClient (but don't alter it)

The Go Driver already supports this:

opts := options.Client().SetHTTPClient(&http.Client{Transport: loghttp.DefaultTransport})

client, err := mongo.Connect(context.Background(), opts)

[jsphpl]

@prestonvasquez I will remove the DefaultHTTPClient and replace it with a NewHTTPClient().

However, I see an issue with the cleanup:

if *c.httpClient == http.Client{} {
	defer httputil.CloseIdleHTTPConnections(c.httpClient)
}

I guess the semantics here are "only clean up if the mongo client owns the http client, i.e. uses its own http.Client instead of a user-provided one". Comparing against the zero http.Client{} will not work to achieve this.

I see two possible alternatives:

1. Clean up unconditionally, even if the client was provided by the user via DI. This is not much worse than status quo, using a singleton http.Client for all mongo client instances, which will get cleaned up when the first mongo client Disconnect()s. Calling CloseIdleConnections() on the transport does not do any real harm – worst case some idle connections need to be re-established during a later request to the same host. This solution is not very clean because if the http.Client was provided via DI, the calling code should be responsible for managing and cleaning up the http.Client. But it is a very simple solution.
2. Memorize whether the http client is user-provided. If we want to stick to only calling CloseIdleConnections() on a client owned (created) by the mongo client itself, we can either
   A) maintain a usingDefaultHttpClient bool in the mongo Client struct and use that as a check in the Disconnect() method. Or we could
   B) maintain a cleanupFuncs []func() error slice, where we register the cleanup function for the http transport when we store the user-provided http client. In Disconnect() we would then loop over that slice and call all the functions in turn.

Which option would you prefer? Am I missing something?

[prestonvasquez]

@jsphpl Suggest retaining the package-level global that the constructor either returns or updates to compare against so we don't have to maintain state / rely on best efforts. Then teardown logic doesn't have to be changed at all.

var DefaultHTTPClient = &http.Client{}

// NewHTTPClient will return the globally-defined DefaultHHTTransport, updating 
// the transport if it differs from the http package DefaultTransport.
func NewHTTPClient() *http.Client {
	client := DefaultHTTPClient
	if _, ok := http.DefaultTransport.(*http.Transport); !ok {
		client.Transport = http.DefaultTransport
	}

	return client
}

func TestDefaultHTTPClientTransport(t *testing.T) {
	t.Run("default", func(t *testing.T) {
		client := NewHTTPClient()

		val := assert.ObjectsAreEqual(http.DefaultClient, client)

		assert.True(t, val)
		assert.Equal(t, DefaultHTTPClient, client)
	})

	t.Run("non-default global transport", func(t *testing.T) {
		http.DefaultTransport = &nonDefaultTransport{}

		client := NewHTTPClient()

		val := assert.ObjectsAreEqual(&nonDefaultTransport{}, client.Transport)

		assert.True(t, val)
		assert.Equal(t, DefaultHTTPClient, client)
		assert.NotEqual(t, http.DefaultClient, client) // Sanity Check
	})
}