Fixes #12348 - HttpClientTransportDynamic does not initialize low-level clients. #12349
Conversation
…el clients. Fixed initialization for HTTP/2 and HTTP/3. Signed-off-by: Simone Bordet <[email protected]>
| * <p>Implementations of this interface are made aware of | ||
| * the {@code HttpClient} instance while it is starting.</p> | ||
| */ |
There was a problem hiding this comment.
Need more info than this. Implementations of this interface that are: set as beans? added as listeners? discovered through ServiceLoader???
I think it should be on all beans (see below)
There was a problem hiding this comment.
Why? There is no need to do that.
There was a problem hiding this comment.
Because Aware is an interface on HttpClient. It looks like a general mechanism, not something just for some transport factories. If it is just for transport factories, then makes it a sub interface of transport factories.
You don't know that transport factories needed this until now, so why not just make it general and any bean added to the client can be aware.
There was a problem hiding this comment.
I have the opposite stance: I don't want to make it a generic mechanism when it is only needed in one case.
We can always make it generic, if we really need to, in the future.
It cannot be made part of ClientConnectionFactory because it's in jetty-io, while awareness is in jetty-client.
There was a problem hiding this comment.
But it is already generic. There is nothing in the name, signature nor javadoc of HttpClient.Aware that says it is not generic and doesn't apply widely.
It is only in the implementation that it is only applied to infos... and I had to look up the code again to see that infos are ClientConnectionFactories.
So you have a generic interface that is not implemented generically and I have to do two dereferences into the implementation to know that.
It is simpler in naming, easier to explain and LESS CODE just to make it generic on all beans rather than specific to just one component.
If you want to keep it like this, then it needs to be renamed to AwareClientConnectionFactory.
| infos.stream() | ||
| .filter(info -> info instanceof HttpClient.Aware) | ||
| .map(HttpClient.Aware.class::cast) | ||
| .forEach(info -> info.setHttpClient(getHttpClient())); |
There was a problem hiding this comment.
infos is an insufficient name. I have to go find its type to understand the code. How about just being verbose clientConnectionFactoryInfos
However, as all the infos are beans anyway, why not look for any contained Aware bean and save some code:
| infos.stream() | |
| .filter(info -> info instanceof HttpClient.Aware) | |
| .map(HttpClient.Aware.class::cast) | |
| .forEach(info -> info.setHttpClient(getHttpClient())); | |
| getContainedBeans(Aware.class).forEach(a -> a.setHttpClient(client)); |
There was a problem hiding this comment.
actuallyu, as getContainerBeans walks the containment tree, you don't have to do this at every level. You can just do this one at the top level and find all Aware components under HttpClient
There was a problem hiding this comment.
infos is an insufficient name. I have to go find its type to understand the code. How about just being verbose clientConnectionFactoryInfos
Why? It is already called ClientConnectionFactory.Info.
We don't duplicate the name of the outer class into inner classes.
There was a problem hiding this comment.
I'm not suggesting changing the name of the type, only the variable
| public void setHttpClient(HttpClient httpClient) | ||
| { | ||
| ClientConnectionFactoryOverHTTP2 factory = (ClientConnectionFactoryOverHTTP2)getClientConnectionFactory(); | ||
| factory.setHttpClient(httpClient); |
| * <p>Implementations of this interface are made aware of | ||
| * the {@code HttpClient} instance while it is starting.</p> | ||
| */ |
There was a problem hiding this comment.
Because Aware is an interface on HttpClient. It looks like a general mechanism, not something just for some transport factories. If it is just for transport factories, then makes it a sub interface of transport factories.
You don't know that transport factories needed this until now, so why not just make it general and any bean added to the client can be aware.
Signed-off-by: Simone Bordet <[email protected]>
…licit. Signed-off-by: Simone Bordet <[email protected]>
...ttp2-tests/src/test/java/org/eclipse/jetty/http2/tests/HttpClientTransportOverHTTP2Test.java
Show resolved
Hide resolved
...ttp3-tests/src/test/java/org/eclipse/jetty/http3/tests/HttpClientTransportOverHTTP3Test.java
Show resolved
Hide resolved
jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/component/ContainerLifeCycle.java
Show resolved
Hide resolved
Signed-off-by: Simone Bordet <[email protected]>
|
After discussion with @lorban, we removed text hinting at ordering from |
sbordet
deleted the
fix/jetty-12.0.x/12348/httpclienttransportdynamic-config
branch
Fixed initialization for HTTP/2 and HTTP/3.