[adev-code]

Motivation and Context

Related to #6013

Docs changed by this PR:

• Javadoc: SdkHttpConfigurationOption.CONNECTION_ACQUIRE_TIMEOUT
• Javadoc: NettyNioAsyncHttpClient.Builder.connectionAcquisitionTimeout
• Source files: SdkHttpConfigurationOption.java, NettyNioAsyncHttpClient.java

CONNECTION_ACQUIRE_TIMEOUT says it is the "Timeout for acquiring an already-established connection from a connection pool". With the Netty client that is not the case: HealthCheckedChannelPool.acquire starts the acquire timer on every acquire, also when a new connection still has to be opened. So when the acquire timeout is shorter than the connection timeout, a request can fail with "Acquire operation took longer than ..." before the connection timeout is reached. The issue shows this with a 5 s acquire timeout and a 10 s connection timeout, and asks that it be called out so users don't set the acquire timeout too small. The same error was reported in jenkinsci/pipeline-aws-plugin#374.

Modifications

• SdkHttpConfigurationOption.CONNECTION_ACQUIRE_TIMEOUT: drops "already-established" and says that, depending on the HTTP client, this time can also include opening a new connection, with Netty as the example.
• NettyNioAsyncHttpClient.Builder.connectionAcquisitionTimeout(Duration): says the time also includes opening a new connection when the pool doesn't have one ready, and to set it to at least connectionTimeout(Duration).

Javadoc only, no behavior change. The issue's second comment (a second request failing about 1 s after the first one) is a behavior question and is left for the team; that's why this PR says "Related to" and not "Fixes".

Testing

• Javadoc only.
• mvn install -pl :http-client-spi,:netty-nio-client -am with tests: http-client-spi passes. netty-nio-client: 452 JUnit + 212 TestNG tests, 0 failures, 0 Checkstyle violations, 0 SpotBugs issues, with ServerConnectivityErrorMessageTest excluded. That test fails the same way (2 of 16) on master without this change in my local macOS environment, so it is not related.
• javadoc -Xdoclint:all on both files: no errors, and the new {@link #connectionTimeout(Duration)} resolves.

License

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

[S-Saranya1]

"Depending on the HTTP client" might leave users unsure which clients count the connection time and which don't. Could we check how each client behaves and say it explicitly? If it's only Netty, we could say "Only the Netty client includes the time to open a new connection", if any client does the same, we can name them. WDYT?

[adev-code]

Good call, thanks. I tested each client with a 2 s acquire timeout and a 6 s connect timeout. Netty and the AWS CRT clients failed after about 2 s with the acquire timeout, so they count the time to open a new connection. The Apache clients and URLConnection failed after about 6 s with the connect timeout, so they don't. In the code, Apache and CRT use this option, and the URLConnection client never reads it. I updated the Javadoc in d1003415d9d to name the clients.

[S-Saranya1]

nit: Thanks for testing all the clients. One small thing since CRT behaves like Netty, could we add the same "set it to at least the connection timeout" note to the CRT builders connectionAcquisitionTimeout docs (AwsCrtHttpClient and AwsCrtAsyncHttpClient)? Otherwise looks good to me.

[github-actions]

This pull request has been closed and the conversation has been locked. Comments on closed PRs are hard for our team to see. If you need more assistance, please open a new issue that references this one.