HTTP Executor has unnecessary channel select in the error handling

[LucasRoesler]

When reviewing a support issue related to the Gateway, I ended up reviewing the HTTP exector to find any context timeouts. It does have a timeout during the proxy to the function implementation, which is fine, but the processing of the error case from the HTTP client does not need the select statement it currently has.

Expected Behaviour

The client error check can be simplified because if the err is already not nil, then either context has already failed or some other error has occurred and we do not need to wait for the timeout. It should be equivalent to use this

if err != nil {

    log.Printf("Upstream HTTP request error: %s\n", err.Error())
    if reqCtx.Err() == context.DeadlineExceeded {
        log.Printf("Upstream HTTP killed due to exec_timeout: %s\n", f.ExecTimeout)
        w.Header().Set("X-Duration-Seconds", fmt.Sprintf("%f", time.Since(startedTime).Seconds()))

        w.WriteHeader(http.StatusGatewayTimeout)
        return nil
    }

    // Error unrelated to context / deadline
    w.Header().Set("X-Duration-Seconds", fmt.Sprintf("%f", time.Since(startedTime).Seconds()))

    w.WriteHeader(http.StatusInternalServerError)

    return nil
}

This is in fact, more explicit and accurate, because it only checks for timeouts and ignores cancels, so the http response code is more accurate. According to the context docs, the context error can be DeadlineExceeded or Canceled, per https://golang.org/pkg/context/#pkg-variables. We either want to handle Canceled separately or treat it as a generic error.

Current Behaviour

When the client errors, we potentially wait for the context timeout, like this

if err != nil {
    log.Printf("Upstream HTTP request error: %s\n", err.Error())


    // Error unrelated to context / deadline
    if reqCtx.Err() == nil {
        w.Header().Set("X-Duration-Seconds", fmt.Sprintf("%f", time.Since(startedTime).Seconds()))


        w.WriteHeader(http.StatusInternalServerError)


        return nil
    }


    select {
    case <-reqCtx.Done():
        {
            if reqCtx.Err() != nil {
                // Error due to timeout / deadline
                log.Printf("Upstream HTTP killed due to exec_timeout: %s\n", f.ExecTimeout)
                w.Header().Set("X-Duration-Seconds", fmt.Sprintf("%f", time.Since(startedTime).Seconds()))


                w.WriteHeader(http.StatusGatewayTimeout)
                return nil
            }


        }
    }

[alexellis]

@LucasRoesler would you raise a PR for this? Can it be tested?