HTTPCLIENT-2385: make DefaultManagedHttpClientConnection and DefaultM…

[winfriedgerlach]

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, 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.

[arturobernalg]

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, 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. With a tiny factory+wrapper you can intercept all requests for logging, metrics, callbacks, etc., while keeping DefaultManagedHttpClientConnection and friends sealed. This avoids bloating the public API and still gives you full customization—no need to make classes non‑final or expose package‑private helpers.

  .setConnectionFactory(new InstrumentedConnFactory()) // your hook
  .build();```

[winfriedgerlach]

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.

                // 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?

[ok2c]

• 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.

[ok2c]

make LoggingInputStream, LoggingOutputStream, and LoggingSocketHolder public

@winfriedgerlach What is the reason those classes should be made public?

[arturobernalg]

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.

                // 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.

[arturobernalg]

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.

                // 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.

[winfriedgerlach]

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.

[ok2c]

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)?

[winfriedgerlach]

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.

[ok2c]

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.

[winfriedgerlach]

@ok2c your wish is my command :-)
changed