Re: [PR] HTTPCLIENT-2385: make DefaultManagedHttpClientConnection and DefaultM… [httpcomponents-client]
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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] HTTPCLIENT-2385: make DefaultManagedHttpClientConnection and DefaultM… [httpcomponents-client]
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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] HTTPCLIENT-2385: make DefaultManagedHttpClientConnection and DefaultM… [httpcomponents-client]
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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] HTTPCLIENT-2385: make DefaultManagedHttpClientConnection and DefaultM… [httpcomponents-client]
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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] HTTPCLIENT-2385: make DefaultManagedHttpClientConnection and DefaultM… [httpcomponents-client]
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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] HTTPCLIENT-2385: make DefaultManagedHttpClientConnection and DefaultM… [httpcomponents-client]
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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] HTTPCLIENT-2385: make DefaultManagedHttpClientConnection and DefaultM… [httpcomponents-client]
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] HTTPCLIENT-2385: make DefaultManagedHttpClientConnection and DefaultM… [httpcomponents-client]
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] HTTPCLIENT-2385: make DefaultManagedHttpClientConnection and DefaultM… [httpcomponents-client]
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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] HTTPCLIENT-2385: make DefaultManagedHttpClientConnection and DefaultM… [httpcomponents-client]
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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] HTTPCLIENT-2385: make DefaultManagedHttpClientConnection and DefaultM… [httpcomponents-client]
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] HTTPCLIENT-2385: make DefaultManagedHttpClientConnection and DefaultM… [httpcomponents-client]
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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
[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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
