Skip to content

Commit

Permalink
[java] Ensure Java 11 client works to support webdriver session
Browse files Browse the repository at this point in the history
  • Loading branch information
@pujagani
pujagani committed Sep 15, 2022
1 parent 7f7199c commit 4671831
Show file tree
Hide file tree
Showing 2 changed files with 18 additions and 4 deletions.
2 changes: 1 addition & 1 deletion java/src/org/openqa/selenium/remote/http/jdk/JdkHttpClient.java
View file
Expand Up @@ -201,7 +201,7 @@ public void close() {
}

private URI getWebSocketUri(HttpRequest request) {
URI uri = messages.createRequest(request).uri();
URI uri = messages.getRawURI(request);
if ("http".equalsIgnoreCase(uri.getScheme())) {
try {
uri = new URI("ws", uri.getUserInfo(), uri.getHost(), uri.getPort(), uri.getPath(), uri.getQuery(), uri.getFragment());
Expand Down
20 changes: 17 additions & 3 deletions java/src/org/openqa/selenium/remote/http/jdk/JdkHttpMessages.java
View file
Expand Up @@ -19,11 +19,14 @@

import org.openqa.selenium.remote.http.AddSeleniumUserAgent;
import org.openqa.selenium.remote.http.ClientConfig;
import org.openqa.selenium.remote.http.Contents;
import org.openqa.selenium.remote.http.HttpMethod;
import org.openqa.selenium.remote.http.HttpRequest;
import org.openqa.selenium.remote.http.HttpResponse;

import java.io.InputStream;
import java.net.URI;
import java.net.URL;
import java.net.URLEncoder;
import java.net.http.HttpRequest.BodyPublishers;
import java.util.Objects;
Expand Down Expand Up @@ -68,18 +71,24 @@ public java.net.http.HttpRequest createRequest(HttpRequest req) {
break;

case POST:
builder = builder.POST(BodyPublishers.ofInputStream(req.getContent()));
break;
// Copy the content into a byte array to avoid reading the content inputstream multiple times.

This comment has been minimized.

Sign in to view

Copy link
@joerg1985

joerg1985 Sep 15, 2022

Member

isn't this the body of the request? i think this should not be red multiple times by the java implementation.

the response body handler is defined in JdkHttpClient#execute

This comment has been minimized.

Sign in to view

Copy link
@pujagani

pujagani Sep 16, 2022

Author Contributor

Selenium has implemented HTTP Requests by creating wrappers internally for it, which is used when creating a webdriver session, to create a request. That is then passed on to the underlying HttpClient implementation. So in that process, it gets read twice. That part of the code is critical in ensuring Selenium functionality is intact.
Not sure I fully understand the concern around the response body handler here.

This comment has been minimized.

Sign in to view

Copy link
@joerg1985

joerg1985 Sep 16, 2022

Member

Thank you for the explanation, i still don't understand how this change reduces the number of reads of the InputStream.
The new code will still read the InputStream (and copy it to byte array), before sending it.
If the InputStream is directly passed the to the BodyPublishers the same should be done by the HttpClient.
A InputStream is consumed after reading it, so the HttpClient should also only be able to read the stream only once. So it just pulls reading the InputStream to another place?

Regarding the reference to the response body: I assumed you are interrested in reading the response body also only once :) and by the way there might be a bug:

The NettyMessages#toSeleniumResponse method ensures the response body can be read multiple times. (By calling the Contents#memoize while translating the response).
I think this is mandatory, e.g. logging could call HttpResponse.toString(), this would read the data and the InputStream would be consumed before passing it back.

If one would like to fix this, i would suggest:
Use the BodyHandlers.ofByteArray() for the jdk response and set the Supplier to () -> new ByteArrayInputStream... so a new ByteArrayInputStream is returned for each call to the supplier.

Kind regards
Jörg

This comment has been minimized.

Sign in to view

Copy link
@pujagani

pujagani Sep 16, 2022

Author Contributor

For the first part, I was seeing an error in the way it was done earlier. The body of the request was not populated when InputStream was being passed directly. Same happened if the InputStream is copied into a ByteArrayInputStream. My understanding was that it may be happening since it was consumed more than once (I can be wrong here since even the ByteArrayInputStream approach did not work).
I debugged and I have observed, it InputStream value is passed on correctly to the Java Http Client, until a point where it all cleared out before sending the body.
Screenshot 2022-09-16 at 2 52 27 PM
After spending spending sufficient time, I have not identified what is causing this. Meanwhile, though it may not reduce the number of reads, but copying into the ByteArray works.

Regarding the response, I appreciate the details shared. Thank you so much. You make a fairpoint. Though I have not seen a bug so far, but yes it should be allowed to be read more than once. I will take a look.

This comment has been minimized.

Sign in to view

Copy link
@pujagani

pujagani Sep 16, 2022

Author Contributor

Appreciate your time here, Selenium project will be happy if you have suggestions and would like to help contribute :)

This comment has been minimized.

Sign in to view

Copy link
@joerg1985

joerg1985 Sep 18, 2022

Member

Could you provide code or infos to reproduce the issue? I am currently at vacation, so it will take some time for me to have a look into it.

The selenium project is really great and i am using it a lot at work and i just try to give something back :)

This comment has been minimized.

Sign in to view

Copy link
@joerg1985

joerg1985 Sep 18, 2022

Member

I think i have found one possible issue with the HttpRequest build in the ProtocolHandshake#createSession. It will return the same InputStream that might be allready consumed, e.g. by a HttpRequest#toString call done by the IDE while debugging. All other areas i have seen (on my smartphone) set a Supplier returning a fresh InputStream on each call. I might be wrong, but it is worth a try to add a call to Contents.memoize in ProtocolHandshake#createSession to ensure the Supplier will return a fresh InputStream on each call and check if your issue is gone. Kind regards Jörg

This comment has been minimized.

Sign in to view

Copy link
@pujagani

pujagani Sep 19, 2022

Author Contributor

I appreciate your feedback here. Thank you for taking the time and effort. I was thinking the same thing over the weekend after our discussion about response handling. I will incorporate your suggestion and give it try and provide an update accordingly.

This comment has been minimized.

Sign in to view

Copy link
@pujagani

pujagani Sep 20, 2022 •
edited

Author Contributor

I tried out the suggestion and it still does not work. I hardcoded a few things to narrow down the problem area.

case POST:
        String huh = "{\n"
                     + "  \"desiredCapabilities\": {\n"
                     + "    \"browserName\": \"MicrosoftEdge\",\n"
                     + "    \"ms:edgeOptions\": {\n"
                     + "      \"args\": [\n"
                     + "      ],\n"
                     + "      \"extensions\": [\n"
                     + "      ]\n"
                     + "    }\n"
                     + "  },\n"
                     + "  \"capabilities\": {\n"
                     + "    \"firstMatch\": [\n"
                     + "      {\n"
                     + "        \"browserName\": \"MicrosoftEdge\",\n"
                     + "        \"ms:edgeOptions\": {\n"
                     + "          \"args\": [\n"
                     + "          ],\n"
                     + "          \"extensions\": [\n"
                     + "          ]\n"
                     + "        }\n"
                     + "      }\n"
                     + "    ]\n"
                     + "  }\n"
                     + "}";
        byte[] sampleData = huh.getBytes();
        // Copy the content into a byte array to avoid reading the content inputstream multiple times.
        builder = builder.POST(BodyPublishers.ofInputStream(() -> new ByteArrayInputStream(sampleData)));
       // builder = builder.POST(BodyPublishers.ofByteArray(sampleData));

Using Inputstream BodyPublisher still did not work but using ByteArray Body Publisher worked. I am trying to see what we are doing wrong.

This comment has been minimized.

Sign in to view

Copy link
@pujagani

pujagani Sep 20, 2022

Author Contributor

It turns out that somehow when Inputstream BodyPublisher is used with EdgeDriver or ChromeDriver, SocketTube(1) got read error: java.io.IOException: connection closed locally message is seen in the debug logs and if it is used with Selenium Grid (which is a netty server) then java.io.UncheckedIOException: java.io.IOException: Connection reset by peer . It works perfectly fine with Firefoxdriver. Based on the errors received, the error is happening due to the receiving server, over which we do not have control. Since the changes made in the PR work in all cases, I am going to let that be.

This comment has been minimized.

Sign in to view

Copy link
@joerg1985

joerg1985 Oct 2, 2022

Member

For the archive, the java 11 client did send a chunked request, this seems not supported by the driver: https://bugs.chromium.org/p/chromedriver/issues/detail?id=1850
Using a BodyPublishers.ofByteArray will ensure the content-length header can be set and no chunking is used.
If one will complain about the increased memory this might be fixed by implementing a custom BodyPublisher implementation. The best solution would be the chromedriver and edgedriver would fix this and the BodyPublishers.ofInputStream could be used.
builder = builder.POST(BodyPublishers.ofByteArray(Contents.bytes(req.getContent())));
break;

case PUT:
builder = builder.PUT(BodyPublishers.ofInputStream(req.getContent()));
builder = builder.PUT(BodyPublishers.ofByteArray(Contents.bytes(req.getContent())));
break;

default:
throw new IllegalArgumentException(String.format("Unsupported request method %s: %s", req.getMethod(), req));
}

for (String name : req.getHeaderNames()) {
// Avoid explicitly setting content-length
// This prevents the IllegalArgumentException that states 'restricted header name: "Content-Length"'
if (name.equals("Content-Length")) {

This comment has been minimized.

Sign in to view

Copy link
@joerg1985

joerg1985 Sep 15, 2022

Member

this should be .equalsIgnoreCase, headers should be written lowercase in future.

This comment has been minimized.

Sign in to view

Copy link
@pujagani

pujagani Sep 16, 2022

Author Contributor

Will update that. Thank you!
continue;
}
for (String value : req.getHeaders(name)) {
builder = builder.header(name, value);
}
Expand All @@ -106,6 +115,11 @@ private String getRawUrl(URI baseUrl, String uri) {
return rawUrl;
}

public URI getRawURI(HttpRequest req) {
String rawUrl = getRawUrl(config.baseUri(), req.getUri());
return URI.create(rawUrl);
}

public HttpResponse createResponse(java.net.http.HttpResponse<InputStream> response) {
HttpResponse res = new HttpResponse();
res.setStatus(response.statusCode());
Expand Down

0 comments on commit 4671831

Please sign in to comment.