Skip to content

GODRIVER-3361 Improve connection error message. #2027

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.

Sign up for GitHub

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 6 commits into from
May 9, 2025
Merged

GODRIVER-3361 Improve connection error message. #2027

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
1cbbbda
GODRIVER-3361 Improve connection error message.
qingyang-hu Apr 25, 2025
076f0df
fix tests
qingyang-hu Apr 28, 2025
9cefb59
minor updates
qingyang-hu Apr 29, 2025
c3a7110
minor updates
qingyang-hu May 2, 2025
482f746
fix test cases
qingyang-hu May 2, 2025
64c75e1
add test cases
qingyang-hu May 9, 2025
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
7 changes: 2 additions & 5 deletions x/mongo/driver/topology/connection.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -212,7 +212,7 @@ func (c *connection) connect(ctx context.Context) (err error) {
// Assign the result of DialContext to a temporary net.Conn to ensure that c.nc is not set in an error case.
tempNc, err := c.config.dialer.DialContext(ctx, c.addr.Network(), c.addr.String())
if err != nil {
return ConnectionError{Wrapped: err, init: true}
return ConnectionError{Wrapped: err, init: true, message: fmt.Sprintf("failed to connect to %s", c.addr)}
}
c.nc = tempNc

Expand All @@ -229,7 +229,7 @@ func (c *connection) connect(ctx context.Context) (err error) {
tlsNc, err := configureTLS(ctx, c.config.tlsConnectionSource, c.nc, c.addr, tlsConfig, ocspOpts)

if err != nil {
return ConnectionError{Wrapped: err, init: true}
return ConnectionError{Wrapped: err, init: true, message: fmt.Sprintf("failed to configure TLS for %s", c.addr)}
}
c.nc = tlsNc
}
Expand Down Expand Up @@ -413,9 +413,6 @@ func (c *connection) readWireMessage(ctx context.Context) ([]byte, error) {
c.close()
}
message := errMsg
if errors.Is(err, io.EOF) {
message = "socket was unexpectedly closed"
}
return nil, ConnectionError{
ConnectionID: c.id,
Wrapped: transformNetworkError(ctx, err, contextDeadlineUsed),
Expand Down
4 changes: 2 additions & 2 deletions x/mongo/driver/topology/connection_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,8 +63,8 @@ func TestConnection(t *testing.T) {
t.Run("connect", func(t *testing.T) {
t.Run("dialer error", func(t *testing.T) {
err := errors.New("dialer error")
var want error = ConnectionError{Wrapped: err, init: true}
conn := newConnection(address.Address(""), WithDialer(func(Dialer) Dialer {
var want error = ConnectionError{Wrapped: err, init: true, message: "failed to connect to testaddr:27017"}
conn := newConnection(address.Address("testaddr"), WithDialer(func(Dialer) Dialer {
return DialerFunc(func(context.Context, string, string) (net.Conn, error) { return nil, err })
}))
got := conn.connect(context.Background())
Expand Down
31 changes: 21 additions & 10 deletions x/mongo/driver/topology/errors.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,10 @@ import (
"context"
"errors"
"fmt"
"io"
"net"
"os"
"strings"
"time"

"go.mongodb.org/mongo-driver/v2/x/mongo/driver/description"
Expand All @@ -28,21 +32,28 @@ type ConnectionError struct {

// Error implements the error interface.
func (e ConnectionError) Error() string {
Copy link
Member

Choose a reason for hiding this comment

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

Suggest adding a compile check for ConnectionError, I just noticed we don't do this:

var _ error = ConnectionError{}

message := e.message
var messages []string
if e.init {
fullMsg := "error occurred during connection handshake"
if message != "" {
fullMsg = fmt.Sprintf("%s: %s", fullMsg, message)
}
message = fullMsg
messages = append(messages, "error occurred during connection handshake")
}
if e.Wrapped != nil && message != "" {
return fmt.Sprintf("connection(%s) %s: %s", e.ConnectionID, message, e.Wrapped.Error())
if e.message != "" {
messages = append(messages, e.message)
}
if e.Wrapped != nil {
return fmt.Sprintf("connection(%s) %s", e.ConnectionID, e.Wrapped.Error())
if errors.Is(e.Wrapped, io.EOF) {
messages = append(messages, "socket was unexpectedly closed")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
messages = append(messages, "socket was unexpectedly closed")
messages = append(messages, "connection closed unexpectedly by the other side")

The ticket suggests "Wrap[ping] io.EOF with an additional error message, like 'connection closed unexpectedly by the other side'". The idea being that we let the user know nothing driver-side is responsible. When the driver closes a connection I would expect something like this: use of closed network connection. Here is a gist with both examples: https://gist.github.com/prestonvasquez/bbab8370950e9c98fa422b6b1ae1069a

}
if errors.Is(e.Wrapped, os.ErrDeadlineExceeded) {
messages = append(messages, "client timed out waiting for server response")
} else if err, ok := e.Wrapped.(net.Error); ok && err.Timeout() {
messages = append(messages, "client timed out waiting for server response")
}
Copy link
Member

Choose a reason for hiding this comment

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

the transformNetworkError function will wrap the error with context.DeadlineExceeded. Should we include that case here for safety?

messages = append(messages, e.Wrapped.Error())
}
if len(messages) > 0 {
return fmt.Sprintf("connection(%s) %s", e.ConnectionID, strings.Join(messages, ": "))
}
return fmt.Sprintf("connection(%s) %s", e.ConnectionID, message)
return fmt.Sprintf("connection(%s)", e.ConnectionID)
}

// Unwrap returns the underlying error.
Expand Down
3 changes: 2 additions & 1 deletion x/mongo/driver/topology/pool_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -471,6 +471,7 @@ func TestPool_checkOut(t *testing.T) {

dialErr := errors.New("create new connection error")
p := newPool(poolConfig{
Address: "testaddr",
ConnectTimeout: defaultConnectionTimeout,
}, WithDialer(func(Dialer) Dialer {
return DialerFunc(func(context.Context, string, string) (net.Conn, error) {
Expand All @@ -481,7 +482,7 @@ func TestPool_checkOut(t *testing.T) {
require.NoError(t, err)

_, err = p.checkOut(context.Background())
var want error = ConnectionError{Wrapped: dialErr, init: true}
var want error = ConnectionError{Wrapped: dialErr, init: true, message: "failed to connect to testaddr:27017"}
assert.Equalf(t, want, err, "should return error from calling checkOut()")
// If a connection initialization error occurs during checkOut, removing and closing the
// failed connection both happen asynchronously with the checkOut. Wait for up to 2s for
Expand Down
2 changes: 1 addition & 1 deletion x/mongo/driver/topology/server_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -376,7 +376,7 @@ func TestServer(t *testing.T) {
}

authErr := ConnectionError{Wrapped: &auth.Error{}, init: true}
netErr := ConnectionError{Wrapped: &net.AddrError{}, init: true}
netErr := ConnectionError{Wrapped: &net.AddrError{}, init: true, message: "failed to connect to localhost:27017"}
for _, tt := range serverTestTable {
t.Run(tt.name, func(t *testing.T) {
var returnConnectionError bool
Expand Down
Loading