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

Misleading docs for HttpClientTransportDynamic #7575

Closed
zUniQueX opened this issue Feb 13, 2022 · 8 comments · Fixed by #7800
Closed

Misleading docs for HttpClientTransportDynamic #7575

zUniQueX opened this issue Feb 13, 2022 · 8 comments · Fixed by #7800

Comments

@zUniQueX
Copy link
Contributor

Target Jetty version(s)
11.0.0 - 11.0.8

Enhancement Description

I wanted to integrate the Jetty HttpClient into a library, that needs to support both protocols HTTP/1.1 and HTTP/2. Therefore I tried using the HttpClientTransportDynamic and provided both ClientConnectionFactory.Info objects as described in the docs.

When trying to request a server that doesn't supports the first protocol in the constructor of the HttpClientTransportDynamic object, the request fails instead of retrying with the other protocol. For example, when prefering HTTP/2 and requesting an HTTP/1.1 only server, the following exception gets thrown:

java.io.IOException: frame_size_error/invalid_frame_length
	at org.eclipse.jetty.http2.HTTP2Session.toFailure(HTTP2Session.java:591)
	at org.eclipse.jetty.http2.HTTP2Session$StreamsState.onSessionFailure(HTTP2Session.java:1948)
	at org.eclipse.jetty.http2.HTTP2Session.onSessionFailure(HTTP2Session.java:536)
	at org.eclipse.jetty.http2.HTTP2Session.onConnectionFailure(HTTP2Session.java:531)
	at org.eclipse.jetty.http2.parser.Parser$Listener$Wrapper.onConnectionFailure(Parser.java:409)
	at org.eclipse.jetty.http2.HTTP2Connection$ParserListener.onConnectionFailure(HTTP2Connection.java:414)
	at org.eclipse.jetty.http2.parser.BodyParser.notifyConnectionFailure(BodyParser.java:218)
	at org.eclipse.jetty.http2.parser.BodyParser.connectionFailure(BodyParser.java:210)
	at org.eclipse.jetty.http2.parser.Parser.connectionFailure(Parser.java:204)
	at org.eclipse.jetty.http2.parser.Parser.parseHeader(Parser.java:150)
	at org.eclipse.jetty.http2.parser.Parser.parse(Parser.java:116)
	at org.eclipse.jetty.http2.HTTP2Connection$HTTP2Producer.produce(HTTP2Connection.java:278)
	at org.eclipse.jetty.util.thread.strategy.AdaptiveExecutionStrategy.produceTask(AdaptiveExecutionStrategy.java:446)
	at org.eclipse.jetty.util.thread.strategy.AdaptiveExecutionStrategy.tryProduce(AdaptiveExecutionStrategy.java:239)
	at org.eclipse.jetty.util.thread.strategy.AdaptiveExecutionStrategy.produce(AdaptiveExecutionStrategy.java:190)
	at org.eclipse.jetty.http2.HTTP2Connection.produce(HTTP2Connection.java:208)
	at org.eclipse.jetty.http2.HTTP2Connection.onFillable(HTTP2Connection.java:155)
	at org.eclipse.jetty.http2.HTTP2Connection$FillableCallback.succeeded(HTTP2Connection.java:378)
	at org.eclipse.jetty.io.FillInterest.fillable(FillInterest.java:100)
	at org.eclipse.jetty.io.SelectableChannelEndPoint$1.run(SelectableChannelEndPoint.java:53)
	at org.eclipse.jetty.util.thread.Invocable.invokeNonBlocking(Invocable.java:151)
	at org.eclipse.jetty.util.thread.strategy.AdaptiveExecutionStrategy.invokeAsNonBlocking(AdaptiveExecutionStrategy.java:429)
	at org.eclipse.jetty.util.thread.strategy.AdaptiveExecutionStrategy.consumeTask(AdaptiveExecutionStrategy.java:371)
	at org.eclipse.jetty.util.thread.strategy.AdaptiveExecutionStrategy.tryProduce(AdaptiveExecutionStrategy.java:268)
	at org.eclipse.jetty.util.thread.strategy.AdaptiveExecutionStrategy.produce(AdaptiveExecutionStrategy.java:190)
	at org.eclipse.jetty.util.thread.QueuedThreadPool.runJob(QueuedThreadPool.java:894)
	at org.eclipse.jetty.util.thread.QueuedThreadPool$Runner.run(QueuedThreadPool.java:1038)
	at java.base/java.lang.Thread.run(Thread.java:829)

I don't know, if I'm using this class correctly but in my opinion the documentation is misleading here.

The typical case is when the server supports both HTTP/1.1 and HTTP/2, but the client does not know that.

If the client doesn't know, which protocols the server supports, the server could only support one of them and therefore the client could eventually fail. Therefore I'd suggest clarifying what this class actually supports and what it doesn't.

Additionally I recognized that the example contains a wrong constant name: HttpClientConnectionFactory.HTTP should be replaced by HttpClientConnectionFactory.HTTP11.

@sbordet
Copy link
Contributor

sbordet commented Feb 14, 2022

@zUniQueX from the stack trace it seems you are using clear-text protocols, so http/1.1 and h2c.

When using clear-text, the client must know a priori what protocol is supported on the server port, because there is no negotiation.

So if you know a server supports only http/1.1 and you send h2c then it's your fault and you get an exception.

Do you have suggestions on how to improve the documentation?

@sbordet
Copy link
Contributor

sbordet commented Feb 14, 2022

Additionally I recognized that the example contains a wrong constant name: HttpClientConnectionFactory.HTTP should be replaced by HttpClientConnectionFactory.HTTP11.

I don't see this. Where it is exactly?

@zUniQueX
Copy link
Contributor Author

@zUniQueX from the stack trace it seems you are using clear-text protocols, so http/1.1 and h2c.

When using clear-text, the client must know a priori what protocol is supported on the server port, because there is no negotiation.

So if you know a server supports only http/1.1 and you send h2c then it's your fault and you get an exception.

Do you have suggestions on how to improve the documentation?

Ah, sorry. I've been using h2 but clear-text HTTP/1.1. Thanks for the explanation.

I would suggest making the docs more explicit, i.e. to provide details on the implementation. For example: Protocol switching can be done from HTTP/1.1 to HTTP/2 using ALPN or using the HTTP Upgrade header for an h2c connection.

@zUniQueX
Copy link
Contributor Author

I don't see this. Where it is exactly?

I meant this line: https://github.com/eclipse/jetty.project/blob/jetty-10.0.x/jetty-client/src/main/java/org/eclipse/jetty/client/dynamic/HttpClientTransportDynamic.java#L58. Thereby the constant HTTP doesn't exist and should be replaced by HTTP11.

sbordet added a commit that referenced this issue Mar 29, 2022
Improved documentation for the clear-text dynamic transport case.
Fixed HttpClientTransportDynamic javadocs.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
@sbordet sbordet linked a pull request Mar 29, 2022 that will close this issue
@sbordet sbordet added this to To do in Jetty 10.0.9/11.0.9 (FROZEN) via automation Mar 29, 2022
@sbordet sbordet moved this from To do to In progress in Jetty 10.0.9/11.0.9 (FROZEN) Mar 29, 2022
@joakime joakime moved this from In progress to Review in progress in Jetty 10.0.9/11.0.9 (FROZEN) Mar 29, 2022
Jetty 10.0.9/11.0.9 (FROZEN) automation moved this from Review in progress to Done Mar 29, 2022
sbordet added a commit that referenced this issue Mar 29, 2022
Improved documentation for the clear-text dynamic transport case.
Fixed HttpClientTransportDynamic javadocs.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
@mperktold
Copy link

I would suggest making the docs more explicit, i.e. to provide details on the implementation. For example: Protocol switching can be done from HTTP/1.1 to HTTP/2 using ALPN or using the HTTP Upgrade header for an h2c connection.

Are there any examples that show how this can be done?

I have the same problem as the OP. I would like to configure an HttpClient such that it uses HTTP/2 if possible, falling back to HTTP/1.1 if needed. I don't need h2c. I tried using HttpClientTransportDynamic for this, but I get the same error.

My intuition was that the client knows that HTTP/2 is only possible with HTTPS. So if I send a request via HTTPS, it could try HTTP/2, but if it's a clear text request, there is no point in even trying.

Also, before looking into this, I expected that the HttpClient would support HTTP/2 out of the box. But then I checked and saw that it always uses HTTP/1.1 with default config. With HttpClientTransportDynamic as configured in the docs, it does use HTTP/2, but then unencrypted requests fail.

Is there a way to configure a single client such that it works for (encrypted) HTTP/2 as well as encrypted and unencrypted HTTP/1.1?

@mperktold
Copy link

OK i digged further into the programming guide an found a solution: by simply reversing the order of h2 and h1, i.e. preferring h1 over h2, everything works as expected:

var clientConnector = new ClientConnector();
var client = new HttpClient(new HttpClientTransportDynamic(
    clientConnector,
    HttpClientConnectionFactory.HTTP11,   // preferred if no ALPN is used
    new ClientConnectionFactoryOverHTTP2.HTTP2(new HTTP2Client(clientConnector))
));

In unencrypted requests where ALPN is not used, the client uses h1.
In encrypted requests, ALPN correctly selects h2, not matter the order in the config.

@sbordet
Copy link
Contributor

sbordet commented May 23, 2024

@mperktold do you have suggestions to make the documentation clearer?

@mperktold
Copy link

You could explicitly state that for the config in the example, clear-text connections will always use h2c, which might not be supported by the server and lead to errors. I would also add that the order can be reversed to use h1 for clear-text connections and still support h2 over TLS for servers that support it.

Now as I understand, h2c is quite rare. I never worked with a server that supported that, let alone required it. If you agree with that assumption, you could first give the example that prefers h1, which should work as expected with most servers. And then you would go on to explain how to use h2c by reversing the order, and maybe even showing that in another example.

More generally, it wasn't clear to me that the order of protocols doesn't matter over TLS, where ALPN takes care of selecting a protocol. My intuition was that I have to put h2 first as I prefer it. But since this preference only applies to TLS, the order doesn't matter. Maybe that could be clarified in the JavaDoc as well.

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

Successfully merging a pull request may close this issue.

3 participants