Skip to content
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.

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

Try to stdlib errors.Unwrap errors as well #1660

Closed
wants to merge 6 commits into from
Closed
Show file tree
Hide file tree
Changes from 4 commits
Commits
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
37 changes: 25 additions & 12 deletions lib/netext/httpext/error_codes.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ package httpext
import (
"crypto/tls"
"crypto/x509"
stderrors "errors"
"fmt"
"net"
"net/url"
Expand Down Expand Up @@ -79,15 +80,15 @@ const (
// errors till 1651 + 13 are other HTTP2 Connection errors with a specific errCode

// Custom k6 content errors, i.e. when the magic fails
//defaultContentError errCode = 1700 // reserved for future use
// defaultContentError errCode = 1700 // reserved for future use
responseDecompressionErrorCode errCode = 1701
)

const (
tcpResetByPeerErrorCodeMsg = "write: connection reset by peer"
tcpResetByPeerErrorCodeMsg = "%s: connection reset by peer"
tcpDialTimeoutErrorCodeMsg = "dial: i/o timeout"
tcpDialRefusedErrorCodeMsg = "dial: connection refused"
tcpBrokenPipeErrorCodeMsg = "write: broken pipe"
tcpBrokenPipeErrorCodeMsg = "%s: broken pipe"
netUnknownErrnoErrorCodeMsg = "%s: unknown errno `%d` on %s with message `%s`"
dnsNoSuchHostErrorCodeMsg = "lookup: no such host"
blackListedIPErrorCodeMsg = "ip is blacklisted"
Expand All @@ -107,7 +108,17 @@ func http2ErrCodeOffset(code http2.ErrCode) errCode {

// errorCodeForError returns the errorCode and a specific error message for given error.
func errorCodeForError(err error) (errCode, string) {
switch e := errors.Cause(err).(type) {
causeErr := errors.Cause(err)

inner := stderrors.Unwrap(causeErr)
if inner != nil && inner != causeErr {
code, resultErr := errorCodeForError(inner)
if code != defaultErrorCode {
return code, resultErr
}
}

switch e := errors.Cause(causeErr).(type) {
case K6Error:
return e.Code, e.Message
case *net.DNSError:
Expand All @@ -133,14 +144,16 @@ func errorCodeForError(err error) (errCode, string) {
// TODO: figure out how this happens
return defaultNetNonTCPErrorCode, err.Error()
}
if e.Op == "write" {
if sErr, ok := e.Err.(*os.SyscallError); ok {
switch sErr.Err {
case syscall.ECONNRESET:
return tcpResetByPeerErrorCode, tcpResetByPeerErrorCodeMsg
case syscall.EPIPE:
return tcpBrokenPipeErrorCode, tcpBrokenPipeErrorCodeMsg
}
if sErr, ok := e.Err.(*os.SyscallError); ok {
switch sErr.Unwrap() {
Copy link
Member

Choose a reason for hiding this comment

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

I am confused - wouldn't this have been unwrapped in the call above? After all, net.OpError has Unwrap(). Maybe that automatic Unwrap() above should only happen in the default: case of this big switch?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here we have syscall.ECONNRESET inside os.SyscallError inside net.OpError maybe inside something else.

I think that for this chain specifically everything implements Unwrap now ... but it didn't when this code was added IIRC. I really don't want to put syscall.ECONNRESET in the top switch because:

  1. that will mean the top switch will be forever long
  2. the windows fix here won't work or will need even MORE working around it >.>
  3. The e.Op will get lost

Moving the Unwrap to the default seems like a not bad idea ... should we also move the Cause then ?

I am not really certain what wraps what and where and whether us moving things around will not break something as ... well most of the tests for this are syntethic - I created a specific wrappings of errors that than goes through this code, but as evident by the fact it hasn't caught some errors ... my matroshkas aren't what is genereated in the actual code of the application. So I do prefer to change this less ... if possible :D

Copy link
Contributor Author

@mstoykov mstoykov Oct 8, 2020

Choose a reason for hiding this comment

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

Moving Unwrap breaks the TestDNsErrorTestDnsResolve as it DNSError inside OpError ... maybe I should just move DNSError under OpError 🤔

Copy link
Member

Choose a reason for hiding this comment

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

Moving the Unwrap to the default seems like a not bad idea ... should we also move the Cause then ?

Yes, this is what I meant, and it is going to make the change more minimal

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unwrap in the begining is the same logic as Cause ... just slower and with a return path:
Cause basically says get me the inner error of my Cause error that doesn't implement the causer interface.
What happens with the Unwrap above is that we Unwrap (and Cause) until we get to an error that no longer implements neither on (or just doesn't have inner error) and than we check it, if it is a concrete error - we return that and if not we go one level above and try that error

Looking at the document Unwrap was added in a later version to github.com/pkg/errors adn arguably if we upgrade the version we can drop the errors.Cause 🤔

case syscall.ECONNRESET:
return tcpResetByPeerErrorCode, fmt.Sprintf(tcpResetByPeerErrorCodeMsg, e.Op)
case syscall.EPIPE:
return tcpBrokenPipeErrorCode, fmt.Sprintf(tcpBrokenPipeErrorCodeMsg, e.Op)
}
code, msg := getOSSyscallErrorCode(e, sErr)
if code != 0 {
return code, msg
}
}
if e.Op == "dial" {
Expand Down
12 changes: 12 additions & 0 deletions lib/netext/httpext/error_codes_syscall_posix.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
// +build !windows

package httpext

import (
"net"
"os"
)

func getOSSyscallErrorCode(e *net.OpError, se *os.SyscallError) (errCode, string) {
return 0, ""
}
16 changes: 16 additions & 0 deletions lib/netext/httpext/error_codes_syscall_windows.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
package httpext

import (
"fmt"
"net"
"os"
"syscall"
)

func getOSSyscallErrorCode(e *net.OpError, se *os.SyscallError) (errCode, string) {
switch se.Unwrap() {
case syscall.WSAECONNRESET:
return tcpResetByPeerErrorCode, fmt.Sprintf(tcpResetByPeerErrorCodeMsg, e.Op)
}
return 0, ""
}
75 changes: 64 additions & 11 deletions lib/netext/httpext/error_codes_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,13 +25,16 @@ import (
"crypto/x509"
"fmt"
"net"
"net/http"
"net/url"
"os"
"runtime"
"syscall"
"testing"
"time"

"github.com/pkg/errors"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"golang.org/x/net/http2"

Expand All @@ -43,9 +46,9 @@ func TestDefaultError(t *testing.T) {
}

func TestHTTP2Errors(t *testing.T) {
var unknownErrorCode = 220
var connectionError = http2.ConnectionError(unknownErrorCode)
var testTable = map[errCode]error{
unknownErrorCode := 220
connectionError := http2.ConnectionError(unknownErrorCode)
testTable := map[errCode]error{
unknownHTTP2ConnectionErrorCode + 1: new(http2.ConnectionError),
unknownHTTP2StreamErrorCode + 1: new(http2.StreamError),
unknownHTTP2GoAwayErrorCode + 1: new(http2.GoAwayError),
Expand All @@ -58,7 +61,7 @@ func TestHTTP2Errors(t *testing.T) {
}

func TestTLSErrors(t *testing.T) {
var testTable = map[errCode]error{
testTable := map[errCode]error{
x509UnknownAuthorityErrorCode: new(x509.UnknownAuthorityError),
x509HostnameErrorCode: new(x509.HostnameError),
defaultTLSErrorCode: new(tls.RecordHeaderError),
Expand All @@ -73,17 +76,17 @@ func TestDNSErrors(t *testing.T) {
)

noSuchHostError.Err = "no such host" // defined as private in go stdlib
var testTable = map[errCode]error{
testTable := map[errCode]error{
defaultDNSErrorCode: defaultDNSError,
dnsNoSuchHostErrorCode: noSuchHostError,
}
testMapOfErrorCodes(t, testTable)
}

func TestBlackListedIPError(t *testing.T) {
var err = netext.BlackListedIPError{}
err := netext.BlackListedIPError{}
testErrorCode(t, blackListedIPErrorCode, err)
var errorCode, errorMsg = errorCodeForError(err)
errorCode, errorMsg := errorCodeForError(err)
require.NotEqual(t, err.Error(), errorMsg)
require.Equal(t, blackListedIPErrorCode, errorCode)
}
Expand All @@ -99,14 +102,14 @@ func (t timeoutError) Error() string {
}

func TestUnknownNetErrno(t *testing.T) {
var err = new(net.OpError)
err := new(net.OpError)
err.Op = "write"
err.Net = "tcp"
err.Err = syscall.ENOTRECOVERABLE // Highly unlikely to actually need to do anything with this error
var expectedError = fmt.Sprintf(
expectedError := fmt.Sprintf(
"write: unknown errno `%d` on %s with message `%s`",
syscall.ENOTRECOVERABLE, runtime.GOOS, err.Err)
var errorCode, errorMsg = errorCodeForError(err)
errorCode, errorMsg := errorCodeForError(err)
require.Equal(t, expectedError, errorMsg)
require.Equal(t, netUnknownErrnoErrorCode, errorCode)
}
Expand All @@ -123,7 +126,7 @@ func TestTCPErrors(t *testing.T) {
notTimeoutedError = &net.OpError{Net: "tcp", Op: "dial", Err: timeoutError(false)}
)

var testTable = map[errCode]error{
testTable := map[errCode]error{
defaultNetNonTCPErrorCode: nonTCPError,
tcpResetByPeerErrorCode: econnreset,
tcpBrokenPipeErrorCode: epipeerror,
Expand Down Expand Up @@ -155,3 +158,53 @@ func testMapOfErrorCodes(t *testing.T, testTable map[errCode]error) {
testErrorCode(t, code, err)
}
}

func TestConnReset(t *testing.T) {
// based on https://gist.github.com/jpittis/4357d817dc425ae99fbf719828ab1800
ln, err := net.Listen("tcp", "localhost:0")
if err != nil {
t.Fatal(err)
}
addr := ln.Addr()
ch := make(chan error, 10)

go func() {
defer close(ch)
// Accept one connection.
conn, innerErr := ln.Accept()
if innerErr != nil {
ch <- innerErr
return
}

// Force an RST
tcpConn := conn.(*net.TCPConn)
innerErr = tcpConn.SetLinger(0)
if innerErr != nil {
ch <- innerErr
}
time.Sleep(time.Second) // Give time for the http request to start
_ = conn.Close()
}()

res, err := http.Get("http://" + addr.String()) //nolint:bodyclose,noctx
require.Nil(t, res)

code, msg := errorCodeForError(err)
assert.Equal(t, tcpResetByPeerErrorCode, code)
assert.Contains(t, msg, fmt.Sprintf(tcpResetByPeerErrorCodeMsg, ""))
for err := range ch {
assert.Nil(t, err)
}
}

func TestDnsResolve(t *testing.T) {
// this uses the Unwrap path
// this is not happening in our current codebase as the resolution in our code
// happens earlier so it doesn't get wrapped, but possibly happens in other cases as well
_, err := http.Get("http://s.com") //nolint:bodyclose,noctx
code, msg := errorCodeForError(err)

assert.Equal(t, dnsNoSuchHostErrorCode, code)
assert.Equal(t, dnsNoSuchHostErrorCodeMsg, msg)
}