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.

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

Adds error code for context deadline exceeded #2008

Merged
merged 6 commits into from
May 19, 2021
Merged

Adds error code for context deadline exceeded #2008

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
2b21665
Adds error code for context deadline exceeded
May 6, 2021
c79562e
Check if resErr is context deadline exceeded in request lifecycle
May 10, 2021
c5edd39
fix tests, use strict timeout check
May 10, 2021
609081f
move error wrapping closer to source
May 11, 2021
87894ce
Code review feedback
May 18, 2021
1675211
Fix build and lint
May 18, 2021
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
2 changes: 1 addition & 1 deletion core/local/local_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1026,7 +1026,7 @@ func TestDNSResolver(t *testing.T) {

expErr := sr(`dial tcp 127.0.0.254:HTTPBIN_PORT: connect: connection refused`)
if runtime.GOOS == "windows" {
expErr = "context deadline exceeded"
expErr = "Request timeout"
}
for name, tc := range testCases {
tc := tc
Expand Down
4 changes: 2 additions & 2 deletions js/modules/k6/http/request_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -326,7 +326,7 @@ func TestRequestAndBatch(t *testing.T) {
`))
endTime := time.Now()
require.Error(t, err)
assert.Contains(t, err.Error(), "context deadline exceeded")
assert.Contains(t, err.Error(), "Request timeout")
assert.WithinDuration(t, startTime.Add(1*time.Second), endTime, 2*time.Second)

logEntry := hook.LastEntry()
Expand All @@ -344,7 +344,7 @@ func TestRequestAndBatch(t *testing.T) {
`))
endTime := time.Now()
require.Error(t, err)
assert.Contains(t, err.Error(), "context deadline exceeded")
assert.Contains(t, err.Error(), "Request timeout")
assert.WithinDuration(t, startTime.Add(1*time.Second), endTime, 2*time.Second)

logEntry := hook.LastEntry()
Expand Down
8 changes: 2 additions & 6 deletions lib/netext/httpext/error_codes.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ const (
// non specific
defaultErrorCode errCode = 1000
defaultNetNonTCPErrorCode errCode = 1010
requestTimeoutErrorCode errCode = 1050
// DNS errors
defaultDNSErrorCode errCode = 1100
dnsNoSuchHostErrorCode errCode = 1101
Expand All @@ -56,7 +57,6 @@ const (
tcpBrokenPipeErrorCode errCode = 1201
netUnknownErrnoErrorCode errCode = 1202
tcpDialErrorCode errCode = 1210
tcpDialTimeoutErrorCode errCode = 1211
tcpDialRefusedErrorCode errCode = 1212
tcpDialUnknownErrnoCode errCode = 1213
tcpResetByPeerErrorCode errCode = 1220
Expand Down Expand Up @@ -86,7 +86,6 @@ const (

const (
tcpResetByPeerErrorCodeMsg = "%s: connection reset by peer"
tcpDialTimeoutErrorCodeMsg = "dial: i/o timeout"
tcpDialRefusedErrorCodeMsg = "dial: connection refused"
tcpBrokenPipeErrorCodeMsg = "%s: broken pipe"
netUnknownErrnoErrorCodeMsg = "%s: unknown errno `%d` on %s with message `%s`"
Expand All @@ -98,6 +97,7 @@ const (
http2ConnectionErrorCodeMsg = "http2: connection error with http2 ErrCode %s"
x509HostnameErrorCodeMsg = "x509: certificate doesn't match hostname"
x509UnknownAuthority = "x509: unknown authority"
requestTimeoutErrorCodeMsg = "Request timeout"
)

func http2ErrCodeOffset(code http2.ErrCode) errCode {
Expand Down Expand Up @@ -140,10 +140,6 @@ func errorCodeForNetOpError(err *net.OpError) (errCode, string) {
}
}

// err.Op is "dial"
if err.Timeout() {
return tcpDialTimeoutErrorCode, tcpDialTimeoutErrorCodeMsg
}
if iErr, ok := err.Err.(*os.SyscallError); ok {
if errno, ok := iErr.Err.(syscall.Errno); ok {
if errno == syscall.ECONNREFUSED ||
Expand Down
2 changes: 0 additions & 2 deletions lib/netext/httpext/error_codes_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,6 @@ func TestTCPErrors(t *testing.T) {
econnrefused = &net.OpError{Net: "tcp", Op: "dial", Err: &os.SyscallError{Err: syscall.ECONNREFUSED}}
errnounknown = &net.OpError{Net: "tcp", Op: "dial", Err: &os.SyscallError{Err: syscall.E2BIG}}
tcperror = &net.OpError{Net: "tcp", Err: errors.New("tcp error")}
timeoutedError = &net.OpError{Net: "tcp", Op: "dial", Err: timeoutError(true)}
notTimeoutedError = &net.OpError{Net: "tcp", Op: "dial", Err: timeoutError(false)}
)

Expand All @@ -141,7 +140,6 @@ func TestTCPErrors(t *testing.T) {
tcpDialUnknownErrnoCode: errnounknown,
defaultTCPErrorCode: tcperror,
tcpDialErrorCode: notTimeoutedError,
tcpDialTimeoutErrorCode: timeoutedError,
}

testMapOfErrorCodes(t, testTable)
Expand Down
5 changes: 5 additions & 0 deletions lib/netext/httpext/transport.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ package httpext

import (
"context"
"errors"
"net"
"net/http"
"net/http/httptrace"
Expand Down Expand Up @@ -243,6 +244,10 @@ func (t *transport) RoundTrip(req *http.Request) (*http.Response, error) {
reqWithTracer := req.WithContext(httptrace.WithClientTrace(ctx, tracer.Trace()))
resp, err := t.state.Transport.RoundTrip(reqWithTracer)

var netError net.Error
if errors.As(err, &netError) && netError.Timeout() {
err = NewK6Error(requestTimeoutErrorCode, requestTimeoutErrorCodeMsg, netError)
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I ended up having to put this in transport instead of request.go. Since transport actually takes care of reporting the error here we need to wrap it goes any further - I tested this with a built CLI to confirm that the error code does propagate all the way to the JS response object.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm we already have something like this in here, but it seems only for dial timeouts: /~https://github.com/k6io/k6/blob/baa5a8af1591ef3c8aa5d5cb1540bceb4a550b6e/lib/netext/httpext/error_codes.go#L143-L146

And it seems likely that your current fix masks that branch, i.e. we'd no longer be able to distinguish between a dial timeout and a timeout after the TCP connection was established. If you move your check in this if body, it will probably also work, without masking dial timeout detection, and while keeping the error detection code in the same place: /~https://github.com/k6io/k6/blob/baa5a8af1591ef3c8aa5d5cb1540bceb4a550b6e/lib/netext/httpext/error_codes.go#L132

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I might be misunderstanding here but I think the err type returned from RoundTrip is a context.deadlineExceededError. This means that it'll never end up making it into errorCodeForNetOpError here. This is also shown by the fact that context deadline exceeded errors return a code of 1000 which is not returned anywhere from errorCodeForNetOpError.

I think that given that these errors will be completely separate times, it should prevent us from masking the dial related errors.

Copy link
Contributor

Choose a reason for hiding this comment

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

The error you create here and then get set in the struct below is the one that then goes through the error_code matching code and net.OpError (the one that is matched in there) does implement net.Error so it will definitely mask it ;).

On the other hand, I am not really certain it is needed, the original code (written by me) was just me trying to match as many errors as we have seen "somehow" which was over a year ago and so the current niceties in the errors package weren't there.

I probably should've had just one timeout error as it is here and had it do everything.

I am not even certain we have had dial timeout error at any point in time or have I just by reading the net code found that this will match something "hopefully" and added it, I certainly tried this for a lot of errors that turned out later weren't matched correctly, because I didn't get the exact wrapping correct and there wasn't errors.As :(.

So I am fine with just dropping the dial timeout error code (and code) and having the rest of the change as it is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok - if we think that creating a generic "timeout" error code is more useful then I've removed the dial conditional of the netOp error branch. Let me know if this isn't what you had in mind.

t.saveCurrentRequest(&unfinishedRequest{
ctx: ctx,
tracer: tracer,
Expand Down