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

Provide a thread-safe way to modify HttpClient proxies at runtime #8723

Closed
cowwoc opened this issue Oct 18, 2022 · 5 comments · Fixed by #9090
Closed

Provide a thread-safe way to modify HttpClient proxies at runtime #8723

cowwoc opened this issue Oct 18, 2022 · 5 comments · Fixed by #9090

Comments

@cowwoc
Copy link
Contributor

cowwoc commented Oct 18, 2022

Jetty version(s)
11.0.12

Enhancement Description
HttpClient.getProxyConfiguration().getProxies() is backed by a ArrayList which is not thread-safe.

Currently, I am forced to externally lock each time I make an HTTP request, or modifying proxies. I am making thousands of requests per second, which means that I need to acquire a lock thousands of times per second.

Possible solutions:

  1. Force users to externally-synchronize access.
  2. Replace ArrayList by LinkedBlockingQueue.
  3. Others???
@sbordet
Copy link
Contributor

sbordet commented Oct 18, 2022

Best would be to deprecate getProxies() and enhance ProxyConfiguration with add() methods and such so that the implementation is not leaked.

@cowwoc want to prepare a PR?

@joakime
Copy link
Contributor

joakime commented Oct 18, 2022

The suggestion from @sbordet is good, but I'm curious ... what is your use case that you need to modify the proxy list so often?

@cowwoc
Copy link
Contributor Author

cowwoc commented Oct 18, 2022

@joakime My proxies are running on AWS spot instances. Spot instances are cheaper, but they can get terminated at any time.

cowwoc added a commit to cowwoc/jetty.project that referenced this issue Oct 18, 2022
cowwoc added a commit to cowwoc/jetty.project that referenced this issue Oct 18, 2022
cowwoc added a commit to cowwoc/jetty.project that referenced this issue Oct 18, 2022
cowwoc added a commit to cowwoc/jetty.project that referenced this issue Oct 18, 2022
@cowwoc
Copy link
Contributor Author

cowwoc commented Oct 18, 2022

I've fixed this differently for version 10/11 and 12 because of the need to maintain backwards compatibility in the former versions. Please take a look:

Version 10,11: #8731
Version 12: #8733

@cowwoc
Copy link
Contributor Author

cowwoc commented Oct 18, 2022

I've run into a bit of a snag. I was only planning to add addProxy(), removeProxy() methods to ProxyConfiguration and remove getProxies(). However, I just ran across ProxyServletTest that invokes client.getProxyConfiguration().getProxies().get(0).getExcludedAddresses().add("127.0.0.1:" + port);.

My line of thinking is that that ProxyConfiguration is there to configure HttpClient. It isn't there to act as a proxy collection for end-users to browse through. If they want to iterate over the list of proxies or look up a proxy by index then they should maintain their own (external) list. Is that a fair assumption? Or do we need to give them more control?

For now, I fixed ProxyServletTest by maintaining an external list of proxies.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants