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.

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

Support ClientProperties.PROXY_URI & al in HttpUrlConnector #5091

Merged
merged 5 commits into from
Jul 1, 2022

Conversation

jbescos
Copy link
Member

@jbescos jbescos commented Jun 22, 2022

Relates to #5089

I don't know what is jnh-connector, Jetty?. That connector is probably not implemented in this PR.

Signed-off-by: Jorge Bescos Gascon <[email protected]>
Signed-off-by: Jorge Bescos Gascon <[email protected]>
@senivam
Copy link
Contributor

senivam commented Jun 22, 2022

jnh-connector (JavaNetHttp connector) relates to Jersey 3.1.+

@jbescos jbescos force-pushed the issue5089 branch 2 times, most recently from 3c609e9 to b258009 Compare June 22, 2022 11:03
@jbescos
Copy link
Member Author

jbescos commented Jun 22, 2022

jnh-connector (JavaNetHttp connector) relates to Jersey 3.1.+

I am going to postpone that connector because I am working in master. After this is merged I will cherry-pick it and add JavaNetHttp for the branch 3.1.

@jbescos jbescos marked this pull request as draft June 22, 2022 11:31
@jbescos
Copy link
Member Author

jbescos commented Jun 22, 2022

There is one issue with the system property 'http.nonProxyHosts' in some connectors.

Internally every connector has its own client (Apache, Jetty, etc), and it is configured in the constructor of the connector. The proxy is part of this configuration. Then the client is re-used in different requests.

'http.nonProxyHosts' depends on runtime data, because we need to check the current URI of the request in order to enable or disable the proxy, but note that the proxy was already configured before in the constructor.

I don't see a good option to address this. In my opinion there are 3:
1 - Do not support any proxy system properties and delegate that on the implementation (Apache, Jetty, etc). In case they don't support it, we don't support it.
2 - In case we really must support this, we could create 2 clients in every connector, one with the proxy and other without proxy. In runtime we select the proper one comparing the host in http.nonProxyHosts with the current request URI.
3 - We can ignore the http.nonProxyHosts in the connectors where there is this issue.

In my opinion we should go for option 1, because option 2 is memory and performance inefficient, and the different implementations can deal with this with no problem because they have access to system properties.

@jbescos jbescos marked this pull request as ready for review June 22, 2022 11:46
Signed-off-by: Jorge Bescos Gascon <[email protected]>
@jbescos jbescos force-pushed the issue5089 branch 5 times, most recently from eda9489 to e30f381 Compare June 27, 2022 06:33
Signed-off-by: Jorge Bescos Gascon <[email protected]>
@jansupol jansupol changed the title Issue5089 Support ClientProperties.PROXY_URI & al in HttpUrlConnector Jul 1, 2022
@jansupol jansupol merged commit a83d288 into eclipse-ee4j:master Jul 1, 2022
@senivam senivam added this to the 2.37 milestone Jul 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants