Fix "http: read on closed response body" error

[Jonathan-Rosenberg]

Closes #8331

Change Description

The current custom error handler is doing redundant work that eventually causes the read on closed response body.

Current Flow

1. Using the retryable client, the generated code issues a request.
2. If the response is retryable, it is retried until the attempts are exhausted.
3. If none of the requests pass, and the error handler isn't nil (it's not), the error handler is called.
4. The custom error handler drains the response's body, closes it, and returns the response along with the passed error (which is nil).
5. The wrapping generated code will parse the response and then it will try to read its body. At this point, it will return the "read on closed response body" error.

Example for generated code from point 5:

// ParsePostStatsEventsResponse parses an HTTP response from a PostStatsEventsWithResponse call
func ParsePostStatsEventsResponse(rsp *http.Response) (*PostStatsEventsResponse, error) {
	bodyBytes, err := ioutil.ReadAll(rsp.Body)
	defer rsp.Body.Close()
	if err != nil {
		return nil, err
	}
.....

Removing the custom error handler will not affect the current usage which will behave as we expect. This is because the behavior of the default retry client is to drain the body and return an error without the response. That way the generated code will not try to read the response, but will return an error...

[github-actions]

E2E Test Results - DynamoDB Local - Local Block Adapter

[github-actions]

E2E Test Results - Quickstart

[N-o-Z]

Why are we not logging the number of attempts anymore?

[Jonathan-Rosenberg]

@N-o-Z the opposite, we now start logging them in case of an error

[N-o-Z]

@N-o-Z the opposite, we now start logging them in case of an error

I might be color blind... 😅

[arielshaqed]

I don't understand why this fixes an issue.

1. The docs explicitly say "If overriding this, be sure to close the body if needed.". We closed the body. How does this create a double-close?

2. The comment for the behaviour of customErrorHandler says:

    // We need to consume response bodies to maintain http connections, but
    // limit the size we consume to respReadLimit.

   IIUC this means that we lose this behaviour. Why is this a good thing? Doesn't it mean retries will be much slower?

Not blocking or approving because I don't understand this PR. Will be happy to change this.

[Jonathan-Rosenberg]

@arielshaqed

1. I think that the notable two words in the provided section in the docs are if needed. We already drain the body and close it in the wrapping generated code. The problem isn't a double close, but a close and then a read by the generated client (as mentioned in the description and shown in the example). The error handler closes the body, and then the generated client tries to read it before it closes it as well (but it fails on the read!).
2. In case of exhausted retries the retryable client itself drains the body with respReadLimit = int64(4096). The provided comment is actually a copy of the code that does that in the retryable client default implementation. The difference is that the retryable client doesn't return a response (as opposed to the custom error handler), but an error (check out the last section in the description after the example)

[arielshaqed]

IIUC the issue is that customErrorHandler - which tries to prevent detailing the error message - has a bug and returns the response.

LGTM!

[Jonathan-Rosenberg]

This is truly a bug, but the bigger picture is that this code is redundant and adds no value.