GODRIVER-2348 Deprecate socketTimeoutMS

internal/integration/client_test.go

@@ -327,7 +327,7 @@ func TestClient(t *testing.T) {
 			// apply the correct URI.
 			invalidClientOpts := options.Client().
 				SetServerSelectionTimeout(100 * time.Millisecond).SetHosts([]string{"invalid:123"}).
-				SetConnectTimeout(500 * time.Millisecond).SetSocketTimeout(500 * time.Millisecond)
+				SetConnectTimeout(500 * time.Millisecond).SetTimeout(500 * time.Millisecond)
 			integtest.AddTestServerAPIVersion(invalidClientOpts)
 			client, err := mongo.Connect(invalidClientOpts)
 			assert.Nil(mt, err, "Connect error: %v", err)

internal/integration/errors_test.go

@@ -104,32 +104,35 @@ func TestErrors(t *testing.T) {
 				"errors.Is failure: expected error %v to be %v", err, context.DeadlineExceeded)
 		})
 
-		mt.Run("socketTimeoutMS timeouts return network errors", func(mt *mtest.T) {
-			_, err := mt.Coll.InsertOne(context.Background(), bson.D{{"x", 1}})
-			assert.Nil(mt, err, "InsertOne error: %v", err)
-
-			// Reset the test client to have a 100ms socket timeout. We do this here rather than passing it in as a
-			// test option using mt.RunOpts because that could cause the collection creation or InsertOne to fail.
-			resetClientOpts := options.Client().
-				SetSocketTimeout(100 * time.Millisecond)
-			mt.ResetClient(resetClientOpts)
-
-			mt.ClearEvents()
-			filter := bson.M{
-				"$where": "function() { sleep(1000); return false; }",
-			}
-			_, err = mt.Coll.Find(context.Background(), filter)
-
-			evt := mt.GetStartedEvent()
-			assert.Equal(mt, "find", evt.CommandName, "expected command 'find', got %q", evt.CommandName)
-
-			assert.False(mt, errors.Is(err, context.DeadlineExceeded),
-				"errors.Is failure: expected error %v to not be %v", err, context.DeadlineExceeded)
-			var netErr net.Error
-			ok := errors.As(err, &netErr)
-			assert.True(mt, ok, "errors.As failure: expected error %v to be a net.Error", err)
-			assert.True(mt, netErr.Timeout(), "expected error %v to be a network timeout", err)
-		})
+		// TODO(GODRIVER-2348): Can we remove this / update it to be specific to
+		// timeoutMS instead of socketTimeoutMS?
+		//mt.Run("socketTimeoutMS timeouts return network errors", func(mt *mtest.T) {
+		//	_, err := mt.Coll.InsertOne(context.Background(), bson.D{{"x", 1}})
+		//	assert.Nil(mt, err, "InsertOne error: %v", err)
+
+		//	// Reset the test client to have a 100ms timeout. We do this here rather than passing it in as a
+		//	// test option using mt.RunOpts because that could cause the collection creation or InsertOne to fail.
+		//	resetClientOpts := options.Client().SetTimeout(100 * time.Millisecond)
+		//	mt.ResetClient(resetClientOpts)
+
+		//	mt.ClearEvents()
+		//	filter := bson.M{
+		//		"$where": "function() { sleep(1000); return false; }",
+		//	}
+		//	_, err = mt.Coll.Find(context.Background(), filter)
+
+		//	fmt.Println("err: ", err)
+
+		//	evt := mt.GetStartedEvent()
+		//	assert.Equal(mt, "find", evt.CommandName, "expected command 'find', got %q", evt.CommandName)
+
+		//	assert.False(mt, errors.Is(err, context.DeadlineExceeded),
+		//		"errors.Is failure: expected error %v to not be %v", err, context.DeadlineExceeded)
+		//	var netErr net.Error
+		//	ok := errors.As(err, &netErr)
+		//	assert.True(mt, ok, "errors.As failure: expected error %v to be a net.Error", err)
+		//	assert.True(mt, netErr.Timeout(), "expected error %v to be a network timeout", err)
+		//})
 	})
 	mt.Run("ServerError", func(mt *mtest.T) {
 		matchWrapped := errors.New("wrapped err")

internal/integration/json_helpers_test.go

@@ -110,9 +110,11 @@ func createClientOptions(t testing.TB, opts bson.Raw) *options.ClientOptions {
 		case "serverSelectionTimeoutMS":
 			sst := convertValueToMilliseconds(t, opt)
 			clientOpts.SetServerSelectionTimeout(sst)
-		case "socketTimeoutMS":
-			st := convertValueToMilliseconds(t, opt)
-			clientOpts.SetSocketTimeout(st)
+		// TODO(GODRIVER-2348): How are we supposed to handle socketTimeoutMS once
+		// deprecated?
+		//case "socketTimeoutMS":
+		//	st := convertValueToMilliseconds(t, opt)
+		//	clientOpts.SetSocketTimeout(st)
 		case "minPoolSize":
 			clientOpts.SetMinPoolSize(uint64(opt.AsInt64()))
 		case "maxPoolSize":

internal/integration/sdam_error_handling_test.go

@@ -49,60 +49,62 @@ func TestSDAMErrorHandling(t *testing.T) {
 	// blockConnection and appName.
 	mt.RunOpts("before handshake completes", baseMtOpts().Auth(true).MinServerVersion("4.4"), func(mt *mtest.T) {
 		mt.RunOpts("network errors", noClientOpts, func(mt *mtest.T) {
-			mt.Run("pool cleared on network timeout", func(mt *mtest.T) {
-				// Assert that the pool is cleared when a connection created by an application
-				// operation thread encounters a timeout caused by socketTimeoutMS during
-				// handshaking.
-
-				appName := "authConnectTimeoutTest"
-				// Set failpoint on saslContinue instead of saslStart because saslStart isn't done when using
-				// speculative auth.
-				mt.SetFailPoint(mtest.FailPoint{
-					ConfigureFailPoint: "failCommand",
-					Mode: mtest.FailPointMode{
-						Times: 1,
-					},
-					Data: mtest.FailPointData{
-						FailCommands:    []string{"saslContinue"},
-						BlockConnection: true,
-						BlockTimeMS:     150,
-						AppName:         appName,
-					},
-				})
-
-				// Reset the client with the appName specified in the failpoint and the pool monitor.
-				tpm := eventtest.NewTestPoolMonitor()
-				mt.ResetClient(baseClientOpts().
-					SetAppName(appName).
-					SetPoolMonitor(tpm.PoolMonitor).
-					// Set a 100ms socket timeout so that the saslContinue delay of 150ms causes a
-					// timeout during socket read (i.e. a timeout not caused by the InsertOne context).
-					SetSocketTimeout(100 * time.Millisecond))
-
-				// Use context.Background() so that the new connection will not time out due to an
-				// operation-scoped timeout.
-				_, err := mt.Coll.InsertOne(context.Background(), bson.D{{"test", 1}})
-				assert.NotNil(mt, err, "expected InsertOne error, got nil")
-				assert.True(mt, mongo.IsTimeout(err), "expected timeout error, got %v", err)
-				assert.True(mt, mongo.IsNetworkError(err), "expected network error, got %v", err)
-				// Assert that the pool is cleared within 2 seconds.
-				assert.Soon(mt, func(ctx context.Context) {
-					ticker := time.NewTicker(100 * time.Millisecond)
-					defer ticker.Stop()
-
-					for {
-						select {
-						case <-ticker.C:
-						case <-ctx.Done():
-							return
-						}
-
-						if tpm.IsPoolCleared() {
-							return
-						}
-					}
-				}, 2*time.Second)
-			})
+			// TODO(GODRIVER-2348): Can we remove this / update it to be specific to
+			// timeoutMS instead of socketTimeoutMS?
+			//mt.Run("pool cleared on network timeout", func(mt *mtest.T) {
+			//	// Assert that the pool is cleared when a connection created by an application
+			//	// operation thread encounters a timeout caused by socketTimeoutMS during
+			//	// handshaking.
+
+			//	appName := "authConnectTimeoutTest"
+			//	// Set failpoint on saslContinue instead of saslStart because saslStart isn't done when using
+			//	// speculative auth.
+			//	mt.SetFailPoint(mtest.FailPoint{
+			//		ConfigureFailPoint: "failCommand",
+			//		Mode: mtest.FailPointMode{
+			//			Times: 1,
+			//		},
+			//		Data: mtest.FailPointData{
+			//			FailCommands:    []string{"saslContinue"},
+			//			BlockConnection: true,
+			//			BlockTimeMS:     150,
+			//			AppName:         appName,
+			//		},
+			//	})
+
+			//	// Reset the client with the appName specified in the failpoint and the pool monitor.
+			//	tpm := eventtest.NewTestPoolMonitor()
+			//	mt.ResetClient(baseClientOpts().
+			//		SetAppName(appName).
+			//		SetPoolMonitor(tpm.PoolMonitor).
+			//		// Set a 100ms socket timeout so that the saslContinue delay of 150ms causes a
+			//		// timeout during socket read (i.e. a timeout not caused by the InsertOne context).
+			//		SetTimeout(100 * time.Millisecond))
+
+			//	// Use context.Background() so that the new connection will not time out due to an
+			//	// operation-scoped timeout.
+			//	_, err := mt.Coll.InsertOne(context.Background(), bson.D{{"test", 1}})
+			//	assert.NotNil(mt, err, "expected InsertOne error, got nil")
+			//	assert.True(mt, mongo.IsTimeout(err), "expected timeout error, got %v", err)
+			//	assert.True(mt, mongo.IsNetworkError(err), "expected network error, got %v", err)
+			//	// Assert that the pool is cleared within 2 seconds.
+			//	assert.Soon(mt, func(ctx context.Context) {
+			//		ticker := time.NewTicker(100 * time.Millisecond)
+			//		defer ticker.Stop()
+
+			//		for {
+			//			select {
+			//			case <-ticker.C:
+			//			case <-ctx.Done():
+			//				return
+			//			}
+
+			//			if tpm.IsPoolCleared() {
+			//				return
+			//			}
+			//		}
+			//	}, 2*time.Second)
+			//})
 
 			mt.RunOpts("pool cleared on non-timeout network error", noClientOpts, func(mt *mtest.T) {
 				mt.Run("background", func(mt *mtest.T) {

internal/integration/unified/client_entity.go

@@ -611,8 +611,10 @@ func setClientOptionsFromURIOptions(clientOpts *options.ClientOptions, uriOpts b
 			clientOpts.SetRetryReads(value.(bool))
 		case "retrywrites":
 			clientOpts.SetRetryWrites(value.(bool))
+		// TODO(GODRIVER-2348): How are we supposed to handle socketTimeoutMS once
+		// deprecated?
 		case "sockettimeoutms":
-			clientOpts.SetSocketTimeout(time.Duration(value.(int32)) * time.Millisecond)
+			clientOpts.SetTimeout(time.Duration(value.(int32)) * time.Millisecond)
 		case "w":
 			wc.W = value
 			wcSet = true

mongo/client.go

@@ -181,6 +181,9 @@ func newClient(opts ...*options.ClientOptions) (*Client, error) {
 		client.retryReads = *clientOpt.RetryReads
 	}
 	// Timeout
+	if to := clientOpt.Timeout; to != nil && *to < 0 {
+		return nil, fmt.Errorf("invalid value for \"timeout\": %q", *to)
+	}
 	client.timeout = clientOpt.Timeout
 	client.httpClient = clientOpt.HTTPClient
 	// WriteConcern

mongo/client_test.go

@@ -501,4 +501,13 @@ func TestClient(t *testing.T) {
 			})
 		}
 	})
+	t.Run("negative timeout will err", func(t *testing.T) {
+		t.Parallel()
+
+		copts := options.Client().SetTimeout(-1 * time.Second)
+		_, err := Connect(copts)
+
+		errmsg := `invalid value for "timeout": "-1s"`
+		assert.Equal(t, errmsg, err.Error(), "expected error %v, got %v", errmsg, err.Error())
+	})
 }

mongo/options/clientoptions.go

@@ -252,13 +252,6 @@ type ClientOptions struct {
 	// Deprecated: This option is for internal use only and should not be set. It may be changed or removed in any
 	// release.
 	Deployment driver.Deployment
-
-	// SocketTimeout specifies the timeout to be used for the Client's socket reads and writes.
-	//
-	// NOTE(benjirewis): SocketTimeout will be deprecated in a future release. The more general Timeout option
-	// may be used in its place to control the amount of time that a single operation can run before returning
-	// an error. Setting SocketTimeout and Timeout on a single client will result in undefined behavior.
-	SocketTimeout *time.Duration
 }
 
 // Client creates a new ClientOptions instance.
@@ -477,10 +470,6 @@ func (c *ClientOptions) ApplyURI(uri string) *ClientOptions {
 		c.ServerSelectionTimeout = &cs.ServerSelectionTimeout
 	}
 
-	if cs.SocketTimeoutSet {
-		c.SocketTimeout = &cs.SocketTimeout
-	}
-
 	if cs.SRVMaxHosts != 0 {
 		c.SRVMaxHosts = &cs.SRVMaxHosts
 	}
@@ -830,29 +819,19 @@ func (c *ClientOptions) SetServerSelectionTimeout(d time.Duration) *ClientOption
 	return c
 }
 
-// SetSocketTimeout specifies how long the driver will wait for a socket read or write to return before returning a
-// network error. This can also be set through the "socketTimeoutMS" URI option (e.g. "socketTimeoutMS=1000"). The
-// default value is 0, meaning no timeout is used and socket operations can block indefinitely.
+// SetTimeout specifies the amount of time that a single operation run on this
+// Client can execute before returning an error. The deadline of any operation
+// run through the Client will be honored above any Timeout set on the Client;
+// Timeout will only be honored if there is no deadline on the operation
+// Context. Timeout can also be set through the "timeoutMS" URI option
+// (e.g. "timeoutMS=1000"). The default value is nil, meaning operations do not
+// inherit a timeout from the Client.
 //
-// NOTE(benjirewis): SocketTimeout will be deprecated in a future release. The more general Timeout option may be used
-// in its place to control the amount of time that a single operation can run before returning an error. Setting
-// SocketTimeout and Timeout on a single client will result in undefined behavior.
-func (c *ClientOptions) SetSocketTimeout(d time.Duration) *ClientOptions {
-	c.SocketTimeout = &d
-	return c
-}
-
-// SetTimeout specifies the amount of time that a single operation run on this Client can execute before returning an error.
-// The deadline of any operation run through the Client will be honored above any Timeout set on the Client; Timeout will only
-// be honored if there is no deadline on the operation Context. Timeout can also be set through the "timeoutMS" URI option
-// (e.g. "timeoutMS=1000"). The default value is nil, meaning operations do not inherit a timeout from the Client.
+// The value for a Timeout must be positive.
 //
-// If any Timeout is set (even 0) on the Client, the values of MaxTime on operation options, TransactionOptions.MaxCommitTime and
-// SessionOptions.DefaultMaxCommitTime will be ignored. Setting Timeout and SocketTimeout or WriteConcern.wTimeout will result
-// in undefined behavior.
-//
-// NOTE(benjirewis): SetTimeout represents unstable, provisional API. The behavior of the driver when a Timeout is specified is
-// subject to change.
+// If any Timeout is set (even 0) on the Client, the values of MaxTime on
+// operation options, TransactionOptions.MaxCommitTime and
+// SessionOptions.DefaultMaxCommitTime will be ignored.
 func (c *ClientOptions) SetTimeout(d time.Duration) *ClientOptions {
 	c.Timeout = &d
 	return c
@@ -1087,9 +1066,6 @@ func MergeClientOptions(opts ...*ClientOptions) *ClientOptions {
 		if opt.Direct != nil {
 			c.Direct = opt.Direct
 		}
-		if opt.SocketTimeout != nil {
-			c.SocketTimeout = opt.SocketTimeout
-		}
 		if opt.SRVMaxHosts != nil {
 			c.SRVMaxHosts = opt.SRVMaxHosts
 		}

mongo/options/clientoptions_test.go

@@ -86,7 +86,6 @@ func TestClientOptions(t *testing.T) {
 			{"RetryWrites", (*ClientOptions).SetRetryWrites, true, "RetryWrites", true},
 			{"ServerSelectionTimeout", (*ClientOptions).SetServerSelectionTimeout, 5 * time.Second, "ServerSelectionTimeout", true},
 			{"Direct", (*ClientOptions).SetDirect, true, "Direct", true},
-			{"SocketTimeout", (*ClientOptions).SetSocketTimeout, 5 * time.Second, "SocketTimeout", true},
 			{"TLSConfig", (*ClientOptions).SetTLSConfig, &tls.Config{}, "TLSConfig", false},
 			{"WriteConcern", (*ClientOptions).SetWriteConcern, writeconcern.Majority(), "WriteConcern", false},
 			{"ZlibLevel", (*ClientOptions).SetZlibLevel, 6, "ZlibLevel", true},
@@ -401,11 +400,6 @@ func TestClientOptions(t *testing.T) {
 				"mongodb://localhost/?serverSelectionTimeoutMS=45000",
 				baseClient().SetServerSelectionTimeout(45 * time.Second),
 			},
-			{
-				"SocketTimeout",
-				"mongodb://localhost/?socketTimeoutMS=15000",
-				baseClient().SetSocketTimeout(15 * time.Second),
-			},
 			{
 				"TLS CACertificate",
 				"mongodb://localhost/?ssl=true&sslCertificateAuthorityFile=testdata/ca.pem",