fix: return errors upon panics or receiving unexpected responses (#433)

This replaces the package-wide logger, used in places like processMessages and reader, and returns instead an error. For backwards compatibility, errors returned from goroutine functions are stored in `Conn.err` and can get retrieved through `Conn.GetLastError`
go-ldap · May 5, 2023 · b50d289 · b50d289
1 parent b0d0dcf
commit b50d289
Show file tree

Hide file tree

Showing 12 changed files with 58 additions and 28 deletions.
diff --git a/add.go b/add.go
@@ -1,6 +1,7 @@
 package ldap
 
 import (
+	"fmt"
 	ber "github.com/go-asn1-ber/asn1-ber"
 )
 
@@ -82,7 +83,7 @@ func (l *Conn) Add(addRequest *AddRequest) error {
 			return err
 		}
 	} else {
-		logger.Printf("Unexpected Response: %d", packet.Children[1].Tag)
+		return fmt.Errorf("ldap: unexpected response: %d", packet.Children[1].Tag)
 	}
 	return nil
 }
diff --git a/client.go b/client.go
@@ -9,7 +9,8 @@ import (
 type Client interface {
 	Start()
 	StartTLS(*tls.Config) error
-	Close()
+	Close() error
+	GetLastError() error
 	IsClosing() bool
 	SetTimeout(time.Duration)
 	TLSConnectionState() (tls.ConnectionState, bool)

diff --git a/conn.go b/conn.go
@@ -111,6 +111,8 @@ type Conn struct {
 	wgClose             sync.WaitGroup
 	outstandingRequests uint
 	messageMutex        sync.Mutex
+
+	err error
 }
 
 var _ Client = &Conn{}
@@ -277,7 +279,7 @@ func (l *Conn) setClosing() bool {
 }
 
 // Close closes the connection.
-func (l *Conn) Close() {
+func (l *Conn) Close() (err error) {
 	l.messageMutex.Lock()
 	defer l.messageMutex.Unlock()
 
@@ -301,13 +303,12 @@ func (l *Conn) Close() {
 		close(l.chanMessage)
 
 		l.Debug.Printf("Closing network connection")
-		if err := l.conn.Close(); err != nil {
-			logger.Println(err)
-		}
-
+		err = l.conn.Close()
 		l.wgClose.Done()
 	}
 	l.wgClose.Wait()
+
+	return err
 }
 
 // SetTimeout sets the time after a request is sent that a MessageTimeout triggers
@@ -323,6 +324,12 @@ func (l *Conn) nextMessageID() int64 {
 	return 0
 }
 
+// GetLastError returns the last recorded error from goroutines like processMessages and reader.
+// Only the last recorded error will be returned.
+func (l *Conn) GetLastError() error {
+	return l.err
+}
+
 // StartTLS sends the command to start a TLS session and then creates a new TLS Client
 func (l *Conn) StartTLS(config *tls.Config) error {
 	if l.isTLS {
@@ -471,7 +478,7 @@ func (l *Conn) sendProcessMessage(message *messagePacket) bool {
 func (l *Conn) processMessages() {
 	defer func() {
 		if err := recover(); err != nil {
-			logger.Printf("ldap: recovered panic in processMessages: %v", err)
+			l.err = fmt.Errorf("ldap: recovered panic in processMessages: %v", err)
 		}
 		for messageID, msgCtx := range l.messageContexts {
 			// If we are closing due to an error, inform anyone who
@@ -520,7 +527,7 @@ func (l *Conn) processMessages() {
 						timer := time.NewTimer(time.Duration(l.requestTimeout))
 						defer func() {
 							if err := recover(); err != nil {
-								logger.Printf("ldap: recovered panic in RequestTimeout: %v", err)
+								l.err = fmt.Errorf("ldap: recovered panic in RequestTimeout: %v", err)
 							}
 
 							timer.Stop()
@@ -542,7 +549,7 @@ func (l *Conn) processMessages() {
 				if msgCtx, ok := l.messageContexts[message.MessageID]; ok {
 					msgCtx.sendResponse(&PacketResponse{message.Packet, nil}, time.Duration(l.requestTimeout))
 				} else {
-					logger.Printf("Received unexpected message %d, %v", message.MessageID, l.IsClosing())
+					l.err = fmt.Errorf("ldap: received unexpected message %d, %v", message.MessageID, l.IsClosing())
 					l.Debug.PrintPacket(message.Packet)
 				}
 			case MessageTimeout:
@@ -569,7 +576,7 @@ func (l *Conn) reader() {
 	cleanstop := false
 	defer func() {
 		if err := recover(); err != nil {
-			logger.Printf("ldap: recovered panic in reader: %v", err)
+			l.err = fmt.Errorf("ldap: recovered panic in reader: %v", err)
 		}
 		if !cleanstop {
 			l.Close()

diff --git a/del.go b/del.go
@@ -1,6 +1,7 @@
 package ldap
 
 import (
+	"fmt"
 	ber "github.com/go-asn1-ber/asn1-ber"
 )
 
@@ -51,7 +52,8 @@ func (l *Conn) Del(delRequest *DelRequest) error {
 			return err
 		}
 	} else {
-		logger.Printf("Unexpected Response: %d", packet.Children[1].Tag)
+		return fmt.Errorf("ldap: unexpected response: %d", packet.Children[1].Tag)
 	}
+
 	return nil
 }
diff --git a/moddn.go b/moddn.go
@@ -1,6 +1,7 @@
 package ldap
 
 import (
+	"fmt"
 	ber "github.com/go-asn1-ber/asn1-ber"
 )
 
@@ -94,7 +95,8 @@ func (l *Conn) ModifyDN(m *ModifyDNRequest) error {
 			return err
 		}
 	} else {
-		logger.Printf("Unexpected Response: %d", packet.Children[1].Tag)
+		return fmt.Errorf("ldap: unexpected response: %d", packet.Children[1].Tag)
 	}
+
 	return nil
 }
diff --git a/modify.go b/modify.go
@@ -2,6 +2,7 @@ package ldap
 
 import (
 	"errors"
+	"fmt"
 
 	ber "github.com/go-asn1-ber/asn1-ber"
 )
@@ -126,8 +127,9 @@ func (l *Conn) Modify(modifyRequest *ModifyRequest) error {
 			return err
 		}
 	} else {
-		logger.Printf("Unexpected Response: %d", packet.Children[1].Tag)
+		return fmt.Errorf("ldap: unexpected response: %d", packet.Children[1].Tag)
 	}
+
 	return nil
 }
 

diff --git a/v3/add.go b/v3/add.go
@@ -1,6 +1,7 @@
 package ldap
 
 import (
+	"fmt"
 	ber "github.com/go-asn1-ber/asn1-ber"
 )
 
@@ -82,7 +83,7 @@ func (l *Conn) Add(addRequest *AddRequest) error {
 			return err
 		}
 	} else {
-		logger.Printf("Unexpected Response: %d", packet.Children[1].Tag)
+		return fmt.Errorf("ldap: unexpected response: %d", packet.Children[1].Tag)
 	}
 	return nil
 }
diff --git a/v3/client.go b/v3/client.go
@@ -9,7 +9,8 @@ import (
 type Client interface {
 	Start()
 	StartTLS(*tls.Config) error
-	Close()
+	Close() error
+	GetLastError() error
 	IsClosing() bool
 	SetTimeout(time.Duration)
 	TLSConnectionState() (tls.ConnectionState, bool)

diff --git a/v3/conn.go b/v3/conn.go
@@ -111,6 +111,8 @@ type Conn struct {
 	wgClose             sync.WaitGroup
 	outstandingRequests uint
 	messageMutex        sync.Mutex
+
+	err error
 }
 
 var _ Client = &Conn{}
@@ -277,7 +279,7 @@ func (l *Conn) setClosing() bool {
 }
 
 // Close closes the connection.
-func (l *Conn) Close() {
+func (l *Conn) Close() (err error) {
 	l.messageMutex.Lock()
 	defer l.messageMutex.Unlock()
 
@@ -301,13 +303,12 @@ func (l *Conn) Close() {
 		close(l.chanMessage)
 
 		l.Debug.Printf("Closing network connection")
-		if err := l.conn.Close(); err != nil {
-			logger.Println(err)
-		}
-
+		err = l.conn.Close()
 		l.wgClose.Done()
 	}
 	l.wgClose.Wait()
+
+	return err
 }
 
 // SetTimeout sets the time after a request is sent that a MessageTimeout triggers
@@ -323,6 +324,12 @@ func (l *Conn) nextMessageID() int64 {
 	return 0
 }
 
+// GetLastError returns the last recorded error from goroutines like processMessages and reader.
+// // Only the last recorded error will be returned.
+func (l *Conn) GetLastError() error {
+	return l.err
+}
+
 // StartTLS sends the command to start a TLS session and then creates a new TLS Client
 func (l *Conn) StartTLS(config *tls.Config) error {
 	if l.isTLS {
@@ -471,7 +478,7 @@ func (l *Conn) sendProcessMessage(message *messagePacket) bool {
 func (l *Conn) processMessages() {
 	defer func() {
 		if err := recover(); err != nil {
-			logger.Printf("ldap: recovered panic in processMessages: %v", err)
+			l.err = fmt.Errorf("ldap: recovered panic in processMessages: %v", err)
 		}
 		for messageID, msgCtx := range l.messageContexts {
 			// If we are closing due to an error, inform anyone who
@@ -520,7 +527,7 @@ func (l *Conn) processMessages() {
 						timer := time.NewTimer(time.Duration(l.requestTimeout))
 						defer func() {
 							if err := recover(); err != nil {
-								logger.Printf("ldap: recovered panic in RequestTimeout: %v", err)
+								l.err = fmt.Errorf("ldap: recovered panic in RequestTimeout: %v", err)
 							}
 
 							timer.Stop()
@@ -542,7 +549,7 @@ func (l *Conn) processMessages() {
 				if msgCtx, ok := l.messageContexts[message.MessageID]; ok {
 					msgCtx.sendResponse(&PacketResponse{message.Packet, nil}, time.Duration(l.requestTimeout))
 				} else {
-					logger.Printf("Received unexpected message %d, %v", message.MessageID, l.IsClosing())
+					l.err = fmt.Errorf("ldap: received unexpected message %d, %v", message.MessageID, l.IsClosing())
 					l.Debug.PrintPacket(message.Packet)
 				}
 			case MessageTimeout:
@@ -569,7 +576,7 @@ func (l *Conn) reader() {
 	cleanstop := false
 	defer func() {
 		if err := recover(); err != nil {
-			logger.Printf("ldap: recovered panic in reader: %v", err)
+			l.err = fmt.Errorf("ldap: recovered panic in reader: %v", err)
 		}
 		if !cleanstop {
 			l.Close()

diff --git a/v3/del.go b/v3/del.go
@@ -1,6 +1,7 @@
 package ldap
 
 import (
+	"fmt"
 	ber "github.com/go-asn1-ber/asn1-ber"
 )
 
@@ -51,7 +52,8 @@ func (l *Conn) Del(delRequest *DelRequest) error {
 			return err
 		}
 	} else {
-		logger.Printf("Unexpected Response: %d", packet.Children[1].Tag)
+		return fmt.Errorf("ldap: unexpected response: %d", packet.Children[1].Tag)
 	}
+
 	return nil
 }
diff --git a/v3/moddn.go b/v3/moddn.go
@@ -1,6 +1,7 @@
 package ldap
 
 import (
+	"fmt"
 	ber "github.com/go-asn1-ber/asn1-ber"
 )
 
@@ -94,7 +95,8 @@ func (l *Conn) ModifyDN(m *ModifyDNRequest) error {
 			return err
 		}
 	} else {
-		logger.Printf("Unexpected Response: %d", packet.Children[1].Tag)
+		return fmt.Errorf("ldap: unexpected response: %d", packet.Children[1].Tag)
 	}
+
 	return nil
 }
diff --git a/v3/modify.go b/v3/modify.go
@@ -2,6 +2,7 @@ package ldap
 
 import (
 	"errors"
+	"fmt"
 
 	ber "github.com/go-asn1-ber/asn1-ber"
 )
@@ -126,8 +127,9 @@ func (l *Conn) Modify(modifyRequest *ModifyRequest) error {
 			return err
 		}
 	} else {
-		logger.Printf("Unexpected Response: %d", packet.Children[1].Tag)
+		return fmt.Errorf("ldap: unexpected response: %d", packet.Children[1].Tag)
 	}
+
 	return nil
 }