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

Ability to construct a detached client Request #7994

Closed
cowwoc opened this issue May 16, 2022 · 9 comments · Fixed by #8103
Closed

Ability to construct a detached client Request #7994

cowwoc opened this issue May 16, 2022 · 9 comments · Fixed by #8103

Comments

@cowwoc
Copy link
Contributor

cowwoc commented May 16, 2022

Target Jetty version(s)
10

Enhancement Description
It would be nice to be able to construct a client-side Request independently of an HttpClient instance.

Use-case:

  • I want to distribute requests among proxies, and explicitly specify which proxy each request is sent to.
  • HttpClient does provide the ability to specify proxies at request-construction time, so I am forced to construct multiple HttpClients (one per proxy).
  • Before choosing which proxy I want to send a request to, I want to look up whether the server response is already cached offline (i.e. in a local database)
  • This means I need to be able to construct a Request independent of a specific HttpClient, look it up in the cache, and if no match is found bind it to a specific HttpClient and send out the request.

As an aside, Apache's HttpClient allows construction and configuration of unbound Request objects. Requests are bound to clients at the very last minute in order to send them out. OkHttp does the same.

It would also be great to be able to specify proxies on a per-Request basis. This would allow me to use a single HttpClient.

@joakime
Copy link
Contributor

joakime commented May 16, 2022

Seems like the Request.tag(String) facilities is what you are looking for.

https://javadoc.io/doc/org.eclipse.jetty/jetty-client/10.0.9/org.eclipse.jetty.client/org/eclipse/jetty/client/api/Request.html

@cowwoc
Copy link
Contributor Author

cowwoc commented May 16, 2022

I'm familiar with setting a request tag in one section of the code and looking it up in a different section, but I don't see how this can help with the above scenario.

I assume you mean using different proxies depending on the Request tag, but I'm not sure how to do that. Can you please clarify what you meant, and possibly provide some example code?

@sbordet
Copy link
Contributor

sbordet commented May 17, 2022

@cowwoc not sure I understand what you're trying to achieve.

From your description you don't need to build a request to look it up in the cache, you just use whatever information you already have (which you would put into a Request object).

If there is a cache hit, you don't build the Request, you just return the server response.

If there is a cache miss, you pick the proxy, which means you pick the client, and you are good.

If the above is correct, then there is no "detach the request" issue.

BTW the request is already "detached", in a way. Try this:

HttpClient client1 = ...;
HttpClient client2 = ...;

Request request1 = client1.newRequest(...);

HttpDestination destination2 = client2.resolveDestination(origin);

destination2.send(request1, result -> { ... });

The above should allow you to send a request with any client.

@sbordet
Copy link
Contributor

sbordet commented May 17, 2022

You can use a tag to differentiate between proxies (you need to write your own Proxy subclass though - I did not try but should be doable, something along the lines of TaggedHttpProxy extends HttpProxy).

The idea is that HttpClient picks the Proxy based on the Origin, which contains the tag.
So you can add N proxies, and each one of them will check if the tag matches.

In pseudo code:

HttpClient client = ...;
List<Proxy> proxies = client.getProxyConfiguration().getProxies();
proxies.add(new TaggedHttpProxy("proxyHost1", proxyPort1, "proxyTag1");
proxies.add(new TaggedHttpProxy("proxyHost2", proxyPort2, "proxyTag2");

client.newRequest("serverHost", serverPort)
    .tag("proxyTag2")
    .send();

The code above should send the request through proxyHost2 (provided TaggedHttpProxy.matches(Origin) is implemented by looking at the Origins tag).

Let us know if it works for you.

@cowwoc
Copy link
Contributor Author

cowwoc commented May 28, 2022

@sbordet I tried implementing the TaggedHttpProxy approach but I think I ran across a bug.

  • I make an HTTP request using a proxy that matches the tag, but the server redirects the client to HTTPS.
  • HttpRedirector.sendRedirect() invokes Request redirect = client.copyRequest(httpRequest, location);
  • However, if you dig into HttpClient.copyRequest() you will find that it does not copy the request tag.

So the proxy gets dropped if the server returns any redirect.

Shouldn't Request.tag() be retained on redirect? I think if you fix that then the approach will work quite nicely.

@cowwoc
Copy link
Contributor Author

cowwoc commented May 28, 2022

The following workaround worked for me and I can now use a single HttpClient with explicit selection between multiple proxies:

HttpClient client = new HttpClient()
{
	protected Request copyRequest(HttpRequest oldRequest, URI newURI)
	{
		return super.copyRequest(oldRequest, newURI).tag(oldRequest.getTag());
	}
};

I'll wait for you to confirm whether this will get fixed in the main codebase. Thanks!

@sbordet
Copy link
Contributor

sbordet commented Jun 2, 2022

@cowwoc I think it's a bug, good catch!

sbordet added a commit that referenced this issue Jun 2, 2022
Implemented copy of the request tag that was mistakenly missing.

Signed-off-by: Simone Bordet <[email protected]>
@sbordet
Copy link
Contributor

sbordet commented Jun 2, 2022

Filed #8103.

@cowwoc
Copy link
Contributor Author

cowwoc commented Jun 2, 2022

Cool, thank you :) You can close this issue now if you want.

@sbordet sbordet closed this as completed in e9262ce Jun 3, 2022
@sbordet sbordet linked a pull request Jun 3, 2022 that will close this issue
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