BUG in main receive loop - it does not finish after TCP connection was closed #84

New issue

Closed

opened 2024-10-18 07:39:51 +00:00 by ksproska · 0 comments

ksproska commented

2024-10-18 07:39:51 +00:00

(Migrated from github.com)

Implementation

In recv() method that is started as a separate goroutine the loop is supposed to stop when io.EOF or io.ErrUnexpectedEOF error is received, however even though connection was successfully closed (for tcp connection) that is not a case and error "use of closed network connection" is printed the whole way through the run of the program suggesting therefore that the recv() method never finishes it's run.

Recognizing connection termination as implemented in apimachinery library

In apimachinery/pkg/util/net/http.go function IsProbableEOF was implemented that besides recognizing connection termination by receiving error io.EOF or io.ErrUnexpectedEOF analysis also other errors that "resemble a connection termination". The function in question:

func IsProbableEOF(err error) bool {
	if err == nil {
		return false
	}
	var uerr *url.Error
	if errors.As(err, &uerr) {
		err = uerr.Err
	}
	msg := err.Error()
	switch {
	case err == io.EOF:
		return true
	case err == io.ErrUnexpectedEOF:
		return true
	case msg == "http: can't write HTTP request on broken connection":
		return true
	case strings.Contains(msg, "http2: server sent GOAWAY and closed the connection"):
		return true
	case strings.Contains(msg, "connection reset by peer"):
		return true
	case strings.Contains(strings.ToLower(msg), "use of closed network connection"):
		return true
	}
	return false
}

Suggested fix

To fix the described bug I would suggest adding either one new condition for "use of closed network connection" error (which is the bug that I found) or to implement the whole function with all listed error messages so that recv method resembles this:

func (s *Session) recv() {
	var err error
	for {
		err = s.recvMsg()
		if IsProbableEOF(err) {
			break
		}
		if err != nil {
			log.Printf("netconf: failed to read incoming message: %v", err)
		}
	}
	// the rest of the method
}

# Implementation In [recv() method](https://github.com/nemith/netconf/blob/main/session.go#L243) that is started as a separate goroutine the loop is supposed to stop when io.EOF or io.ErrUnexpectedEOF error is received, however even though connection was successfully closed (for tcp connection) that is not a case and error "use of closed network connection" is printed the whole way through the run of the program suggesting therefore that the recv() method never finishes it's run. # Recognizing connection termination as implemented in apimachinery library In [apimachinery/pkg/util/net/http.go function IsProbableEOF](https://github.com/kubernetes/apimachinery/blob/v0.31.1/pkg/util/net/http.go#L79) was implemented that besides recognizing connection termination by receiving error io.EOF or io.ErrUnexpectedEOF analysis also other errors that "resemble a connection termination". The function in question: ```go func IsProbableEOF(err error) bool { if err == nil { return false } var uerr *url.Error if errors.As(err, &uerr) { err = uerr.Err } msg := err.Error() switch { case err == io.EOF: return true case err == io.ErrUnexpectedEOF: return true case msg == "http: can't write HTTP request on broken connection": return true case strings.Contains(msg, "http2: server sent GOAWAY and closed the connection"): return true case strings.Contains(msg, "connection reset by peer"): return true case strings.Contains(strings.ToLower(msg), "use of closed network connection"): return true } return false } ``` # Suggested fix To fix the described bug I would suggest adding either one new condition for "use of closed network connection" error (which is the bug that I found) or to implement the whole function with all listed error messages so that recv method resembles this: ```go func (s *Session) recv() { var err error for { err = s.recvMsg() if IsProbableEOF(err) { break } if err != nil { log.Printf("netconf: failed to read incoming message: %v", err) } } // the rest of the method } ```