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

[🚀 Feature]: Need a way to enforce HTTP1.1 usage with the Java HTTP client #12918

Closed
mykola-mokhnach opened this issue Oct 10, 2023 · 17 comments

Comments

@mykola-mokhnach
Copy link

Feature and motivation

Currently it is not possible to enforce HTTP1.1 usage in the client. This feature is needed because Appium server only supports HTTP1.1 so far (SPDY support has been added recently, but it's optional), so we'd like to default to HTTP1.1 for good.

Usage example

Not sure yet where exactly this needs to live.

@github-actions
Copy link

@mykola-mokhnach, thank you for creating this issue. We will troubleshoot it as soon as we can.


Info for maintainers

Triage this issue by using labels.

If information is missing, add a helpful comment and then I-issue-template label.

If the issue is a question, add the I-question label.

If the issue is valid but there is no time to troubleshoot it, consider adding the help wanted label.

If the issue requires changes or fixes from an external project (e.g., ChromeDriver, GeckoDriver, MSEdgeDriver, W3C), add the applicable G-* label, and it will provide the correct link and auto-close the issue.

After troubleshooting the issue, please add the R-awaiting answer label.

Thank you!

@titusfortner
Copy link
Member

I started working on this, looks straightforward, hope to have something we can put in nightly

@joerg1985
Copy link
Member

I think this is nothing the client should do.

The inital request is done with http 1.1 in any case and will ony upgrade to http/2 when the server will respond with switch protocol.

The server should remove the http Upgrade header (not the one for WebSockets ;)) to avoid the http/2 upgrade.

@mykola-mokhnach
Copy link
Author

mykola-mokhnach commented Oct 11, 2023

I think this is nothing the client should do.

The inital request is done with http 1.1 in any case and will ony upgrade to http/2 when the server will respond with switch protocol.

The server should remove the http Upgrade header (not the one for WebSockets ;)) to avoid the http/2 upgrade.

maybe. What I observe for now is that we did not see connection issues with previous client versions using netty @asolntsev got a similar results with their tests. And enforcing HTTP version was the only workaround I was able to google.
@joerg1985 I would be happy to accept any suggestions to avoid the aforementioned connection errors that don't require this particular change.

@joerg1985
Copy link
Member

@mykola-mokhnach I just had a look at the initial request and it is like expected.
You can enable the logging by using System.setProperty("jdk.httpclient.HttpClient.log", "headers,content");

The only difference in the request are the 'Connection', 'HTTP2-Settings', 'Upgrade' headers.

HTTP2 enabled client:

POST /session HTTP/1.1
Connection: Upgrade, HTTP2-Settings
Content-Length: 215
Host: 127.0.0.1:5555
HTTP2-Settings: AAEAAEAAAAIAAAABAAMAAABkAAQBAAAAAAUAAEAA
Upgrade: h2c
Cache-Control: no-cache
Content-Type: application/json; charset=utf-8
User-Agent: selenium/4.14.0 (java windows)

HTTP2 disabled client:

POST /session HTTP/1.1
Content-Length: 215
Host: 127.0.0.1:5555
Cache-Control: no-cache
Content-Type: application/json; charset=utf-8
User-Agent: selenium/4.14.0 (java windows)

So i assume either the Appium http server or one of the drivers are not able to process this headers.
If the driver fails processing the headers the Appium http server could filter these headers.
The ReverseProxyHandler of the Selenium server does filter these and other headers.
Is this done by the Appium server too or are the headers just passed to the driver?

If you need to have a workaround for now, it might be possible to create a system property that selenium will read and force the http1.1 version. @diemol @titusfortner what do you think about this? this should be removed in selenium 5 and is only a workaround for this issue

@titusfortner
Copy link
Member

As I see it:

  1. Fix Appium http server
  2. Have Java Appium require the usage of the JDK HTTP Client specifically and force the version
  3. System Property that toggles the version in the JDK HTTP Client that Appium can set as default
  4. Add version to the Client Config and have the JDK client use it if found ([java] allow setting version in the Http Client Config #12919)

We should at least file an issue for the first one

I prefer 4 to 3 from the Selenium side.

@mykola-mokhnach
Copy link
Author

As I see it:

  1. Fix Appium http server
  2. Have Java Appium require the usage of the JDK HTTP Client specifically and force the version
  3. System Property that toggles the version in the JDK HTTP Client that Appium can set as default
  4. Add version to the Client Config and have the JDK client use it if found ([java] allow setting version in the Http Client Config #12919)

We should at least file an issue for the first one

I prefer 4 to 3 from the Selenium side.

#1 can hardly be fixed now as ExpressJS that we use to serve HTTP does not have HTTP2 support and it is not very clear when it is going to have it.

@titusfortner
Copy link
Member

Is there any consideration of switching off ExpressJS or using spdy package with it?

@mykola-mokhnach
Copy link
Author

switching off ExpressJS

this would be a huge refactoring without any visible improvements and multiple breaking changes

using spdy package with it

The last commit to spdy package happened 2 years ago. It has been incorporated recently, although is only used if server-side https is enabled. It's too much of risk to enable it by default for plain http.

cc @jlipps

@joerg1985
Copy link
Member

@mykola-mokhnach i do not think the appium server must support http/2 to fix this. The upgrade to http/2 is only done by the client when the server does answer with
switch protocol
It is more about removing some headers from the request send to the Appium server before passing them to the drivers. The selenium server is doing this in the ReverseProxyHandler.

So again the question, does the appium server allready remove these headers from the request before sending it to the driver?

@mykola-mokhnach
Copy link
Author

If this is only about removing particular headers then I could try something like appium/java-client@924838f

@joerg1985
Copy link
Member

These headers are added by the java http client automatically, there is no way to suppress this on the client side. These headers are part of the protocol and the http client does not allow to break the protocol.

The filter must be added to the server side, i think there is no other way to handle this.

@mykola-mokhnach
Copy link
Author

mykola-mokhnach commented Oct 11, 2023

Then I would rather use the solution provided by @titusfortner
Let say I apply the changes to the server now:

  • Existing versions of it still won't have them. I don't expect all users to upgrade their server after the client upgrade
  • There is still no guarantee about the client behavior after this change is applied

@joerg1985
Copy link
Member

I agree you need a workaround to this for some time in any case.

@titusfortner
Copy link
Member

Is what was implemented in Selenium in 4.14.1 sufficient to close this?

@mykola-mokhnach
Copy link
Author

yes, I would say it is ok now.
thanks for your efforts.

Copy link

github-actions bot commented Dec 7, 2023

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked and limited conversation to collaborators Dec 7, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants