Skip to content

Commit

Permalink
bug fix http request pre closure (#851)
Browse files Browse the repository at this point in the history
* big fix http request pre closure

* bug fix http request pre-closure
  • Loading branch information
bishnuag authored Jan 31, 2023
1 parent 8e827df commit 2262fbc
Show file tree
Hide file tree
Showing 4 changed files with 85 additions and 11 deletions.
29 changes: 19 additions & 10 deletions runtime/client_http_request.go
Original file line number Diff line number Diff line change
Expand Up @@ -215,11 +215,11 @@ func (req *ClientHTTPRequest) Do() (*ClientHTTPResponse, error) {
var retryCount int64 = 1
var res *http.Response

//when timeoutAndRetryOptions per request is not configured, use default client level timeout
// when timeoutAndRetryOptions per request is not configured, use default client level timeout
if req.timeoutAndRetryOptions == nil || req.timeoutAndRetryOptions.MaxAttempts == 0 {
res, err = req.client.Client.Do(req.httpReq.WithContext(ctx))
} else {
res, retryCount, err = req.executeDoWithRetry(ctx) //new code for retry and timeout per ep level
res, retryCount, err = req.executeDoWithRetry(ctx) // new code for retry and timeout per ep level
}

span.Finish()
Expand All @@ -237,7 +237,7 @@ func (req *ClientHTTPRequest) Do() (*ClientHTTPResponse, error) {
return req.res, nil
}

//executeDoWithRetry will execute executeDo with retries
// executeDoWithRetry will execute executeDo with retries
func (req *ClientHTTPRequest) executeDoWithRetry(ctx context.Context) (*http.Response, int64, error) {
var err error
var res *http.Response
Expand All @@ -264,26 +264,35 @@ func (req *ClientHTTPRequest) executeDoWithRetry(ctx context.Context) (*http.Res
zap.Int("maxAttempts", req.timeoutAndRetryOptions.MaxAttempts),
zap.Bool("shouldRetry", shouldRetry))

//TODO (future releases) - make retry conditional, inspect error/response and then retry
// TODO (future releases) - make retry conditional, inspect error/response and then retry

//reassign body
// reassign body
if req.rawBody != nil && len(req.rawBody) > 0 {
req.httpReq.Body = io.NopCloser(bytes.NewBuffer(req.rawBody))
}

//Break loop if no retries
// Break loop if no retries
if !shouldRetry {
break
}
}
return nil, retryCount, err
}

//executeDo will send the request out with a timeout
// executeDo will send the request out with a timeout
func (req *ClientHTTPRequest) executeDo(ctx context.Context) (*http.Response, error) {
attemptCtx, cancelFn := context.WithTimeout(ctx, req.timeoutAndRetryOptions.RequestTimeoutPerAttemptInMs)
defer cancelFn()
return req.client.Client.Do(req.httpReq.WithContext(attemptCtx))
ctx, cancel := context.WithTimeout(ctx, req.timeoutAndRetryOptions.RequestTimeoutPerAttemptInMs)
defer cancel()
res, err := req.client.Client.Do(req.httpReq.WithContext(ctx))
// when no error, read body and capture before closing the connection
if err == nil {
req.res.setRawHTTPResponse(res)
_, err = req.res.ReadAll()
if err != nil {
return nil, err
}
}
return res, err
}

// InjectSpanToHeader will inject span to request header
Expand Down
54 changes: 54 additions & 0 deletions runtime/client_http_request_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -270,6 +270,60 @@ func TestMakingClientCallWithHeadersWithRequestLevelTimeoutAndRetries(t *testing
"the request should have failed due to context deadline (timeout)")
}

func TestMakingClientCallWithHeadersWithRequestLevelTimeoutAndRetriesSuccess(t *testing.T) {
gateway, err := benchGateway.CreateGateway(
defaultTestConfig,
defaultTestOptions,
exampleGateway.CreateGateway,
)
if !assert.NoError(t, err) {
return
}
defer gateway.Close()

bgateway := gateway.(*benchGateway.BenchGateway)

bgateway.HTTPBackends()["bar"].HandleFunc(
"POST", "/bar-path",
func(w http.ResponseWriter, r *http.Request) {
bodyBytes, _ := io.ReadAll(r.Body)
w.WriteHeader(200)
response := map[string]string{"Example-Header": r.Header.Get("Example-Header"), "body": string(bodyBytes)}
responseBytes, _ := json.Marshal(response)
_, _ = w.Write(responseBytes)
// Check that the default header got set and actually sent to the server.
assert.Equal(t, r.Header.Get("X-Client-ID"), "bar")
assert.Equal(t, r.Header.Get("Accept"), "application/test+json")
},
)

deps := bgateway.Dependencies.(*exampleGateway.DependenciesTree)
barClient := deps.Client.Bar
client := barClient.HTTPClient()

retryOptionsCopy := retryOptions
retryOptionsCopy.RequestTimeoutPerAttemptInMs = 500 * time.Millisecond
retryOptionsCopy.MaxAttempts = 2

ctx := context.Background()
req := zanzibar.NewClientHTTPRequest(ctx, "bar", "Normal", "bar::Normal", client, &retryOptionsCopy)

err = req.WriteJSON(
"POST",
client.BaseURL+"/bar-path",
map[string]string{
"Example-Header": "Example-Value",
"Accept": "application/test+json",
},
"dummy body",
)
assert.NoError(t, err)

_, err = req.Do()

assert.NoError(t, err)
}

func TestBarClientWithoutHeaders(t *testing.T) {
gateway, err := benchGateway.CreateGateway(
defaultTestConfig,
Expand Down
11 changes: 11 additions & 0 deletions runtime/client_http_response.go
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ type ClientHTTPResponse struct {
finishTime time.Time
finished bool
rawResponse *http.Response
responseRead bool
rawResponseBytes []byte
StatusCode int
Duration time.Duration
Expand All @@ -68,6 +69,16 @@ func (res *ClientHTTPResponse) setRawHTTPResponse(httpRes *http.Response) {

// ReadAll reads bytes from response.
func (res *ClientHTTPResponse) ReadAll() ([]byte, error) {
// if response is already read
if res.responseRead {
return res.GetRawBody(), nil
}

// mark response read to true
defer func() {
res.responseRead = true
}()

rawBody, err := io.ReadAll(res.rawResponse.Body)

cerr := res.rawResponse.Body.Close()
Expand Down
2 changes: 1 addition & 1 deletion scripts/cover.sh
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,7 @@ cat ./coverage/istanbul.json | jq '[
] | from_entries' > ./coverage/istanbul-runtime.json
echo "Checking code coverage for runtime folder"
./node_modules/.bin/istanbul check-coverage --statements 99.51 \
./node_modules/.bin/istanbul check-coverage --statements 99.45 \
./coverage/istanbul-runtime.json
cat ./coverage/istanbul.json | jq '[
Expand Down

0 comments on commit 2262fbc

Please sign in to comment.