Add more tests to verify connection closure and small code clean up

[zoewangg]

Description

Reviewed all use of connection closure and verified the resources are being properly released/closed/shutdown after each request as well as after invoke client#close.

This PR added more tests to verify the correctness and did some small code cleanup.

Summary of Connection Closure

Per Request

• Sync path:

Sockets are being closed by closing the underlying InputStream via HandleResponseStage#closeInputStreamIfNeeded unless needsConnectionLeftOpen is true (default to false).

For streaming requests, if a customized ResponseHandler is provided with needsConnectionLeftOpen overriden to true, the underlying connection will not be closed.

• Async path:

Channels are being released after each request in netty module unless keep alive is true via ResponseHandler#finalizeRequest (I renamed it to finalizeResponse in this PR)

Per Client After invoking Client#close

• SDK

  • the thread pools will be shutdown and all Closable will be closed via AmazonSyncHttpClient#close and AmazonAsyncHttpClient#close
  • the underlying http client will be closed unless it's a customer provided client.

• ApacheHttpClient:
  HttpClientConnectionManager will be shutdown and IdleConnectionReaper will be cleaned up.

• NettyNioAyncHttpClient
  EventLoopGroup will be shutdown and channel pools will be closed

• UrlConnectionHttpClient
  No op. Nothing to close

Let me know if I missed anything.

License

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

[codecov-io]

Codecov Report

Merging #1034 into master will increase coverage by 0.07%.
The diff coverage is 89.47%.

@@             Coverage Diff              @@
##             master    #1034      +/-   ##
============================================
+ Coverage     55.57%   55.65%   +0.07%     
- Complexity     4590     4598       +8     
============================================
  Files           815      815              
  Lines         27906    27918      +12     
  Branches       2246     2246              
============================================
+ Hits          15509    15538      +29     
+ Misses        11676    11658      -18     
- Partials        721      722       +1

Impacted Files	Coverage Δ	Complexity Δ
.../http/apache/internal/ApacheHttpRequestConfig.java	96.42% <ø> (ø)	7 <0> (ø)	⬇️
...dk/http/urlconnection/UrlConnectionHttpClient.java	81.25% <ø> (ø)	10 <0> (ø)	⬇️
...rnal/http/pipeline/stages/HandleResponseStage.java	80.48% <ø> (ø)	11 <0> (ø)	⬇️
...awssdk/http/nio/netty/NettyNioAsyncHttpClient.java	66.36% <100%> (+2.28%)	17 <2> (+1)	⬆️
...wssdk/http/nio/netty/internal/ResponseHandler.java	62.83% <100%> (ø)	13 <0> (ø)	⬇️
...re/amazon/awssdk/http/apache/ApacheHttpClient.java	62.42% <100%> (+3.67%)	18 <0> (+1)	⬆️
...ttp/pipeline/stages/MakeAsyncHttpRequestStage.java	79.16% <33.33%> (ø)	16 <0> (ø)	⬇️
...are/amazon/awssdk/core/internal/util/Mimetype.java	78.04% <0%> (-2.44%)	14% <0%> (-1%)
...rnal/http/pipeline/stages/AsyncRetryableStage.java	85.5% <0%> (+2.89%)	3% <0%> (ø)	⬇️
... and 5 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 89826f1...980b09d. Read the comment docs.

[dagnir]

nit: any reason not to call super.close()?

[zoewangg]

Per javadoc, ByteArrayInputStream#close has no effect.

Closing a {@code ByteArrayInputStream} has no effect.

[dagnir]

What do you think about adding a similar test where we return the inputStream, and then call close() and then check to see that the wrapped InputStream is closed?

We could look into a similar test for async

[zoewangg]

+1, will add that test.

I thought about adding a similar test for async but unlike sync where connections are being closed by closing the inputStream, channels are being released in the netty module so there's no easy way to verify that here.