Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

HTTPCLIENT-2321 Clarify default implementation of HttpClientConnectio… #553

Closed
wants to merge 1 commit into from
Closed

HTTPCLIENT-2321 Clarify default implementation of HttpClientConnectio… #553

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 22 additions & 1 deletion httpclient5/src/main/java/org/apache/hc/client5/http/impl/classic/HttpClientBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@
import org.apache.hc.client5.http.impl.auth.BearerSchemeFactory;
import org.apache.hc.client5.http.impl.auth.DigestSchemeFactory;
import org.apache.hc.client5.http.impl.auth.SystemDefaultCredentialsProvider;
import org.apache.hc.client5.http.impl.io.PoolingHttpClientConnectionManager;
import org.apache.hc.client5.http.impl.io.PoolingHttpClientConnectionManagerBuilder;
import org.apache.hc.client5.http.impl.routing.DefaultProxyRoutePlanner;
import org.apache.hc.client5.http.impl.routing.DefaultRoutePlanner;
Expand Down Expand Up @@ -134,7 +135,27 @@
* exclusive and may not apply when building {@link CloseableHttpClient}
* instances.
* </p>
*
* The following are the default implementations per component
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kwin In all honesty next time I am going to touch HttpClientBuilder I am not sure I will get around to updating this table. It will ultimately rot and will cause more harm than good.

In any case If you want this change-set committed, please at the very least update HttpAsyncClientBuilder, CachingHttpClientBuilder and CachingHttpAsyncClientBuilder

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this is a lot of effort. The other alternative would be to establish a common naming (e.g. always use Default prefix, but this won't be backwards compatible) or at least clarify in the javadoc of the implementation whether it is the default one or not. WDYT?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kwin As I mentioned in the JIRA I am not in favor of documenting the defaults in javadocs because some people might see them as a part of public contract but you do it consistently across the code base I will review and commit the PR.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You already have that all over the place:

  1. httpcomponents-client/httpclient5/src/main/java/org/apache/hc/client5/http/impl/auth/BasicAuthCache.java

    Line 49 in f2b9a37

    * Default implementation of {@link AuthCache}. This implements

  2. httpcomponents-client/httpclient5/src/main/java/org/apache/hc/client5/http/cookie/BasicCookieStore.java

    Line 45 in f2b9a37

    * Default implementation of {@link CookieStore}

  3. httpcomponents-client/httpclient5/src/main/java/org/apache/hc/client5/http/impl/auth/BasicCredentialsProvider.java

    Line 40 in f2b9a37

    * Default implementation of {@link CredentialsStore}.

  4. httpcomponents-client/httpclient5/src/main/java/org/apache/hc/client5/http/impl/DefaultRedirectStrategy.java

    Line 50 in f2b9a37

    * Default implementation of {@link RedirectStrategy}.

  5. httpcomponents-client/httpclient5/src/main/java/org/apache/hc/client5/http/impl/DefaultConnectionKeepAliveStrategy.java

    Line 45 in f2b9a37

    * Default implementation of a strategy deciding duration

    ....

And yes, you are right, I consider this default part of the contract (i.e. changing it is possible but only in a minor version explicitly stating the change somewhere)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kwin Those are the default impl of an interface. They have nothing to do with the behavior of client builders.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Except for the last they are all implementations of an interface which can be set on the HttpClientBuilder. Therefore I assume that the default applies here also.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kwin I will have to close this PR unless the proposed changes can be made consistently across the entire code base.

* <table>
* <tr><th>Component</th><th>Default implementation</th></tr>
* <tr><td>Request executor</td><td>{@link HttpRequestExecutor}</td></tr>
* <tr><td>Connection manager</td><td>{@link PoolingHttpClientConnectionManager}</td></tr>
* <tr><td>Connection reuse strategy</td><td>{@link DefaultClientConnectionReuseStrategy}</td></tr>
* <tr><td>Connection keep-alive strategy</td><td>{@link DefaultConnectionKeepAliveStrategy}</td></tr>
* <tr><td>Route planner</td><td>{@link DefaultRoutePlanner}</td></tr>
* <tr><td>Redirect strategy</td><td>{@link DefaultRedirectStrategy}</td></tr>
* <tr><td>Target authentication strategy</td><td>{@link DefaultAuthenticationStrategy}</td></tr>
* <tr><td>Proxy authentication strategy</td><td>{@link DefaultAuthenticationStrategy}</td></tr>
* <tr><td>User token handler</td><td>{@link DefaultUserTokenHandler}</td></tr>
* <tr><td>Retry strategy</td><td>{@link DefaultHttpRequestRetryStrategy}</td></tr>
* <tr><td>Cookie store</td><td>{@link BasicCookieStore}</td></tr>
* <tr><td>Credentials provider</td><td>{@link BasicCredentialsProvider} or {@link SystemDefaultCredentialsProvider}</td></tr>
* <tr><td>Backoff manager</td><td>not set</td></tr>
* <tr><td>Connection backoff strategy</td><td>not set</td></tr>
* <tr><td>Redirect strategy</td><td>{@link DefaultRedirectStrategy}</td></tr>
* <tr><td>Route planner</td><td>One of {@link DefaultProxyRoutePlanner}, {@link SystemDefaultRoutePlanner} or {@link DefaultRoutePlanner}</td></tr>
* <tr><td>Scheme port resolver</td><td>{@link DefaultSchemePortResolver}</td></tr>
* </table>
* @since 4.3
*/
public class HttpClientBuilder {
Expand Down
3 changes: 2 additions & 1 deletion .../src/main/java/org/apache/hc/client5/http/impl/io/PoolingHttpClientConnectionManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,8 @@
import org.slf4j.LoggerFactory;

/**
* {@code ClientConnectionPoolManager} maintains a pool of
* {@code PoolingHttpClientConnectionManager} is the default implementation of
* {@link HttpClientConnectionManager}. It maintains a pool of
* {@link ManagedHttpClientConnection}s and is able to service connection requests
* from multiple execution threads. Connections are pooled on a per route
* basis. A request for a route which already the manager has persistent
Expand Down
Loading