You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hc.apache.org by "winfriedgerlach (via GitHub)" <gi...@apache.org> on 2025/07/29 16:13:02 UTC
[PR] HTTPCLIENT-2385: make DefaultManagedHttpClientConnection and DefaultM… [httpcomponents-client]
winfriedgerlach opened a new pull request, #692:
URL: https://github.com/apache/httpcomponents-client/pull/692
Cf. https://issues.apache.org/jira/browse/HTTPCLIENT-2385:
I'd like to improve extensibility of `DefaultManagedHttpClientConnection` and `DefaultManagedAsyncClientConnection`. My request somehow resembles the very old issue [HTTPCLIENT-1374](https://issues.apache.org/jira/browse/HTTPCLIENT-1374), in which `DefaultManagedHttpClientConnection` was initially made public.
IMHO, implementations named `Default...` should both demonstrate best practices of API usage and act as easily modifiable blueprints for HttpClient users. This is currently not the case, because both `DefaultManagedHttpClientConnection` and `DefaultManagedAsyncClientConnection` are final and `DefaultManagedHttpClientConnection` uses package-private classes (Logging...*).
So if you want to write your own, slightly modified `MyManagedHttpClientConnection` (e.g., change some logging or call a callback when a method is invoked), you will need to extend the super class `DefaultBHttpClientConnection` (which is public non-final), copy a lot of code from `DefaultManagedHttpClientConnection` and possibly even create your own copies of `LoggingInputStream`, `LoggingOutputStream`, and `LoggingSocketHolder`. Something that could be done with very few lines of code, maybe even in an anonymous class, has then become multiple classes with several hundred LoC to review and maintain (especially if `DefaultBHttpClientConnection` or `DefaultManagedHttpClientConnection` are changed in future versions of HttpClient).
I suggest to
* make `DefaultManagedHttpClientConnection` and `DefaultManagedAsyncClientConnection` public non-final
* make `LoggingInputStream`, `LoggingOutputStream`, and `LoggingSocketHolder` public
This shouldn't hurt anyone (doesn't break APIs) and help some users like our project.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
Re: [PR] HTTPCLIENT-2385: make DefaultManagedHttpClientConnection and DefaultM… [httpcomponents-client]
Posted by "arturobernalg (via GitHub)" <gi...@apache.org>.
arturobernalg commented on PR #692:
URL: https://github.com/apache/httpcomponents-client/pull/692#issuecomment-3136876685
> Hi @arturobernalg, I am already using a custom connection factory to create my "instrumented connections". But that was not the question that I wanted to answer with this PR: I wanted to make the creation of such "custom connections" easier.
>
> Take this example: if I principally appreciate the behavior of `DefaultManagedHttpClientConnection` and it was not final, I could easily add stuff to the log when closing a connection.
>
> ```java
> // in ManagedHttpClientConnectionFactory#createConnection:
> final var connection = new DefaultManagedHttpClientConnection(id) {
> @Override
> public void close() throws IOException {
> var certificate = (X509Certificate) getSSLSession().getPeerCertificates()[0];
> var dn = certificate.getSubjectX500Principal().getName();
> LOG.info("Closing connection {} with DN: {}", id, dn);
> super.close();
> }
> };
> ```
Use the `HttpConnectionFactory` SPI with an anonymous decorator around `DefaultManagedHttpClientConnection` to override `close()` for your logging.
That way you don’t expose internals and keep the API sealed while getting the extension you need.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
Re: [PR] HTTPCLIENT-2385: make DefaultManagedHttpClientConnection and DefaultM… [httpcomponents-client]
Posted by "ok2c (via GitHub)" <gi...@apache.org>.
ok2c commented on PR #692:
URL: https://github.com/apache/httpcomponents-client/pull/692#issuecomment-3139095303
> > > make LoggingInputStream, LoggingOutputStream, and LoggingSocketHolder public
> >
> >
> > @winfriedgerlach What is the reason those classes should be made public?
>
> OK @ok2c , I have reverted this change. I guess just having `DefaultManagedHttpClientConnection` public would do the job for me.
@winfriedgerlach Can you also live with those classes being internal (annotated `@Internal`)?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
Re: [PR] HTTPCLIENT-2385: make DefaultManagedHttpClientConnection and DefaultM… [httpcomponents-client]
Posted by "ok2c (via GitHub)" <gi...@apache.org>.
ok2c merged PR #692:
URL: https://github.com/apache/httpcomponents-client/pull/692
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
Re: [PR] HTTPCLIENT-2385: make DefaultManagedHttpClientConnection and DefaultM… [httpcomponents-client]
Posted by "winfriedgerlach (via GitHub)" <gi...@apache.org>.
winfriedgerlach commented on PR #692:
URL: https://github.com/apache/httpcomponents-client/pull/692#issuecomment-3140878878
@ok2c your wish is my command :-)
changed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
Re: [PR] HTTPCLIENT-2385: make DefaultManagedHttpClientConnection and DefaultM… [httpcomponents-client]
Posted by "ok2c (via GitHub)" <gi...@apache.org>.
ok2c commented on PR #692:
URL: https://github.com/apache/httpcomponents-client/pull/692#issuecomment-3136513807
> make LoggingInputStream, LoggingOutputStream, and LoggingSocketHolder public
@winfriedgerlach What is the reason those classes should be made public?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
Re: [PR] HTTPCLIENT-2385: make DefaultManagedHttpClientConnection and DefaultM… [httpcomponents-client]
Posted by "winfriedgerlach (via GitHub)" <gi...@apache.org>.
winfriedgerlach commented on PR #692:
URL: https://github.com/apache/httpcomponents-client/pull/692#issuecomment-3135887395
Hi @arturobernalg, I am already using a custom connection factory to create my "instrumented connections". But that was not the question that I wanted to answer with this PR: I wanted to make the creation of such "custom connections" easier.
Take this example: if I principally appreciate the behavior of `DefaultManagedHttpClientConnection` and it was not final, I could easily add stuff to the log when closing a connection.
```java
// in ManagedHttpClientConnectionFactory#createConnection:
final var connection = new DefaultManagedHttpClientConnection(id) {
@Override
public void close() throws IOException {
var certificate = (X509Certificate) getSSLSession().getPeerCertificates()[0];
var dn = certificate.getSubjectX500Principal().getName();
LOG.info("Closing connection {} with DN: {}", id, dn);
super.close();
}
};
```
(just as an example, don't worry about missing checks etc.)
What would be your alternative solution to this approach?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
Re: [PR] HTTPCLIENT-2385: make DefaultManagedHttpClientConnection and DefaultM… [httpcomponents-client]
Posted by "arturobernalg (via GitHub)" <gi...@apache.org>.
arturobernalg commented on PR #692:
URL: https://github.com/apache/httpcomponents-client/pull/692#issuecomment-3136965295
> > Hi @arturobernalg, I am already using a custom connection factory to create my "instrumented connections". But that was not the question that I wanted to answer with this PR: I wanted to make the creation of such "custom connections" easier.
> > Take this example: if I principally appreciate the behavior of `DefaultManagedHttpClientConnection` and it was not final, I could easily add stuff to the log when closing a connection.
> > ```java
> > // in ManagedHttpClientConnectionFactory#createConnection:
> > final var connection = new DefaultManagedHttpClientConnection(id) {
> > @Override
> > public void close() throws IOException {
> > var certificate = (X509Certificate) getSSLSession().getPeerCertificates()[0];
> > var dn = certificate.getSubjectX500Principal().getName();
> > LOG.info("Closing connection {} with DN: {}", id, dn);
> > super.close();
> > }
> > };
> > ```
>
> @winfriedgerlach Use the `HttpConnectionFactory` SPI with an anonymous decorator around `DefaultManagedHttpClientConnection` to override `close()` for your logging. That way you don’t expose internals and keep the API sealed while getting the extension you need.
@winfriedgerlach, to build on that IMO, exposing logging internals like `LoggingInputStream` could introduce unintended side effects in performance or even security, as users might override them in ways that disrupt stream handling or leak data.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
Re: [PR] HTTPCLIENT-2385: make DefaultManagedHttpClientConnection and DefaultM… [httpcomponents-client]
Posted by "ok2c (via GitHub)" <gi...@apache.org>.
ok2c commented on PR #692:
URL: https://github.com/apache/httpcomponents-client/pull/692#issuecomment-3140797330
> > > > > make LoggingInputStream, LoggingOutputStream, and LoggingSocketHolder public
> > > >
> > > >
> > > > @winfriedgerlach What is the reason those classes should be made public?
> > >
> > >
> > > OK @ok2c , I have reverted this change. I guess just having `DefaultManagedHttpClientConnection` public would do the job for me.
> >
> >
> > @winfriedgerlach Can you also live with those classes being internal (annotated `@Internal`)?
>
> @ok2c I don't have an opinion on that one.
@winfriedgerlach Let me re-phrase. If you add the annotation I will approve the PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
Re: [PR] HTTPCLIENT-2385: make DefaultManagedHttpClientConnection and DefaultM… [httpcomponents-client]
Posted by "winfriedgerlach (via GitHub)" <gi...@apache.org>.
winfriedgerlach commented on PR #692:
URL: https://github.com/apache/httpcomponents-client/pull/692#issuecomment-3139518521
> > > > make LoggingInputStream, LoggingOutputStream, and LoggingSocketHolder public
> > >
> > >
> > > @winfriedgerlach What is the reason those classes should be made public?
> >
> >
> > OK @ok2c , I have reverted this change. I guess just having `DefaultManagedHttpClientConnection` public would do the job for me.
>
> @winfriedgerlach Can you also live with those classes being internal (annotated `@Internal`)?
@ok2c I don't have an opinion on that one.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
Re: [PR] HTTPCLIENT-2385: make DefaultManagedHttpClientConnection and DefaultM… [httpcomponents-client]
Posted by "arturobernalg (via GitHub)" <gi...@apache.org>.
arturobernalg commented on PR #692:
URL: https://github.com/apache/httpcomponents-client/pull/692#issuecomment-3133421996
> Cf. https://issues.apache.org/jira/browse/HTTPCLIENT-2385:
>
> I'd like to improve extensibility of `DefaultManagedHttpClientConnection` and `DefaultManagedAsyncClientConnection`. My request somehow resembles the very old issue [HTTPCLIENT-1374](https://issues.apache.org/jira/browse/HTTPCLIENT-1374), in which `DefaultManagedHttpClientConnection` was initially made public.
>
> IMHO, implementations named `Default...` should both demonstrate best practices of API usage and act as easily modifiable blueprints for HttpClient users. This is currently not the case, because both `DefaultManagedHttpClientConnection` and `DefaultManagedAsyncClientConnection` are final and `DefaultManagedHttpClientConnection` uses package-private classes (`Logging...`).
>
> So if you want to write your own, slightly modified `MyManagedHttpClientConnection` (e.g., change some logging or call a callback when a method is invoked), you will need to extend the super class `DefaultBHttpClientConnection` (which is public non-final), copy a lot of code from `DefaultManagedHttpClientConnection` and possibly even create your own copies of `LoggingInputStream`, `LoggingOutputStream`, and `LoggingSocketHolder`. Something that could be done with very few lines of code, maybe even in an anonymous class, has then become multiple classes with several hundred LoC to review and maintain (especially if `DefaultBHttpClientConnection` or `DefaultManagedHttpClientConnection` are changed in future versions of HttpClient).
>
> I suggest to
>
> * make `DefaultManagedHttpClientConnection` and `DefaultManagedAsyncClientConnection` public non-final
>
> * make `LoggingInputStream`, `LoggingOutputStream`, and `LoggingSocketHolder` public
>
>
> This shouldn't hurt anyone (doesn't break APIs) and help some users like our project.
@winfriedgerlach, IMO Rather than opening up those impl classes, you can get the same extensibility via the public HttpConnectionFactory SPI:
```PoolingHttpClientConnectionManagerBuilder.create()
.setConnectionFactory(new InstrumentedConnFactory()) // your hook
.build();```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
Re: [PR] HTTPCLIENT-2385: make DefaultManagedHttpClientConnection and DefaultM… [httpcomponents-client]
Posted by "ok2c (via GitHub)" <gi...@apache.org>.
ok2c commented on PR #692:
URL: https://github.com/apache/httpcomponents-client/pull/692#issuecomment-3136306127
> * make `DefaultManagedHttpClientConnection` and `DefaultManagedAsyncClientConnection` public non-final
@winfriedgerlach I do not have a problem with making `DefaultManagedHttpClientConnection` and `DefaultManagedAsyncClientConnection` public non0final as long as they are kept internal.
I am substantially less thrilled about the possibility of opening up anything logging related.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
Re: [PR] HTTPCLIENT-2385: make DefaultManagedHttpClientConnection and DefaultM… [httpcomponents-client]
Posted by "winfriedgerlach (via GitHub)" <gi...@apache.org>.
winfriedgerlach commented on PR #692:
URL: https://github.com/apache/httpcomponents-client/pull/692#issuecomment-3138863137
> > make LoggingInputStream, LoggingOutputStream, and LoggingSocketHolder public
>
> @winfriedgerlach What is the reason those classes should be made public?
OK @ok2c , I have reverted this change. I guess having `DefaultManagedHttpClientConnection` public would do the job for me.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org