Fix for S3Client with httpClient as url-connection-client fails with EOFException when executing HeadObjectRequest for gzip encoded object

[joviegas]

Motivation and Context

• Fix for S3Client with httpCLient as url-connection-client fails with EOFException when executing HeadObjectRequest for gzip encoded object
• Please refer S3Client fails with EOFException when executing HeadObjectRequest for gzip encoded object #2259

Modifications

• Similar to other http client where the responseBody is set to Null when the response doesnot have stream in its entity or in other words if http response doesnot have payload.

• Refererence

• Async Htttp client ==>Treat zero-byte responses from HTTP clients as not having content when it comes to creating an SDK HTTP full request. #2201

• Sync Http Client code ==> ApacheHttpClient_sdk_code

    private HttpExecuteResponse createResponse(org.apache.http.HttpResponse apacheHttpResponse,
                                               HttpRequestBase apacheRequest) throws IOException {

        // ....existing code...
        AbortableInputStream responseBody = apacheHttpResponse.getEntity() != null ?
                                   toAbortableInputStream(apacheHttpResponse, apacheRequest) : null;

        return HttpExecuteResponse.builder().response(responseBuilder.build()).responseBody(responseBody).build();

    }

Testing

• Added new Junit
• Perform all the integ test of S3 by changing the Httpclient as UrlHttpClient and all the test passes.

Screenshots (if appropriate)

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)

Checklist

License

• I confirm that this pull request can be released under the Apache 2 license

[millems]

Should we do this in tryGetErrorStream and tryGetInputStream (or their downstream methods)? It seems weird that we have responseContent being present even when there's technically no content.

[joviegas]

• Agree, thats because of https://github.com/JetBrains/jdk8u_jdk/blob/master/src/share/classes/sun/net/www/protocol/http/HttpURLConnection.java#L1858
• Thus it make sense in adding in tryGetInputStream
• However in case of tryGetErrorStream we get null Stream in case of errors for all type of method like 'GET' , 'HEAD' etc , so I think it not required for tryGetErrorStream

[millems]

Is it possible that we might need to check for no-content in tryGetErrorStream?

[joviegas]

It might be possible, but will do some deep dive , also will need to add a test case for this . Since I am going on leave will close this PR for now and will raise a separate PR when back from leave , will keep this task is in fast track

[joviegas]

As per the code walk we donot need this check for erroStreams , details in #3363

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

100.0% Coverage
0.0% Duplication