Do not set content length to zero

[krmahadevan]

Fixes: #11342

Thanks for contributing to Selenium!
A PR well described will help maintainers to quickly review and merge it

Before submitting your PR, please check our contributing guidelines.
Avoid large PRs, help reviewers by making them as simple and short as possible.

Description

Motivation and Context

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Checklist

• I have read the contributing document.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have added tests to cover my changes.
• All new and existing tests passed.

[krmahadevan]

@pujagani @diemol - I haven't been able to figure out a proper way to test this functionality manually because I basically need to create selenium/standalone-firefox docker image with the newly built jar and then use that.

I attempted the following but this generates an error saying

grid-node-docker-1    | 06:36:35.008 WARN [SessionSlot.apply] - Unable to create session
grid-node-docker-1    | java.lang.IllegalArgumentException: Assets path must be set
grid-node-docker-1    |         at org.openqa.selenium.internal.Require.nonNull(Require.java:59)
grid-node-docker-1    |         at org.openqa.selenium.grid.node.docker.DockerSession.<init>(DockerSession.java:61)
grid-node-docker-1    |         at org.openqa.selenium.grid.node.docker.DockerSessionFactory.apply(DockerSessionFactory.java:269)
grid-node-docker-1    |         at org.openqa.selenium.grid.node.docker.DockerSessionFactory.apply(DockerSessionFactory.java:90)
grid-node-docker-1    |         at org.openqa.selenium.grid.node.local.SessionSlot.apply(SessionSlot.java:147)
grid-node-docker-1    |         at org.openqa.selenium.grid.node.local.LocalNode.newSession(LocalNode.java:382)
grid-node-docker-1    |         at org.openqa.selenium.grid.node.NewNodeSession.execute(NewNodeSession.java:52)
grid-node-docker-1    |         at org.openqa.selenium.remote.http.Route$TemplatizedRoute.handle(Route.java:192)

Steps:

1. Built the grid jar (bazel build grid)
2. cloned the docker selenium project
3. copied the selenium grid jar to the Base folder in the docker selenium project
4. Altered the Dockerfile in this directory to refer to the local jar instead of downloading it from Maven central.
5. Built all the images by running VERSION=local make build
6. Altered the docker compose file and the config.toml (references of them are available in the defect ) to refer to the local images.
7. Started the grid via docker compose
8. Ran a sample test that looks like below

  public static void main(String[] args) throws MalformedURLException {
    RemoteWebDriver driver = null;
    try {
      driver = new RemoteWebDriver(new URL("http://localhost:4444"), new FirefoxOptions());
      driver.get("http://www.google.com");
      System.err.println("Title " + driver.getTitle());
    } finally {
      if (driver != null) {
        driver.quit();
      }
    }
  }

[krmahadevan]

Ok please ignore the error msg.. That was my mistake... I didnt add a volume for mapping the assets in my docker compose file.

Once I added it

    volumes:
      - ./config.toml:/opt/bin/config.toml
      - ./assets:/opt/selenium/assets

It worked fine.. a test runs fine.. That PR is now tested out.. I am able to run a firefox test inside a dynamic grid without any issues.

[jsa34]

Came across this, too, after updating thinking the issue was resolved with latest release. Could this be reviewed/merged/released ASAP?

[diemol]

Thank you, @krmahadevan!

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

[krmahadevan]

@diemol @pujagani - Please help re-open this defect. It looks like this is still NOT done.

Here's what I have as analysis:

1. We got rid of all places wherein we are setting the content length explicitly to zero.
2. When starting a container, we use the Docker container API with a POST message, but it has no body and so its content length now gets evaluated to zero.
3. We are using the JDK client by default for all of our http client needs.
4. We create a HttpRequest via org.openqa.selenium.remote.http.jdk.JdkHttpMessages#createRequest which internally calls org.openqa.selenium.remote.http.jdk.JdkHttpMessages#notChunkingBodyPublisher for a POST message.
5. Since as per the docker api spec, there's no payload for this request, we end up getting evaluated to a content length of zero.
6. This in turn triggers the validation check at the JDK HttpClient level saying

grid-node-docker-1    | java.lang.IllegalArgumentException: non-positive contentLength: 0
grid-node-docker-1    |         at java.net.http/java.net.http.HttpRequest$BodyPublishers.fromPublisher(HttpRequest.java:539)
grid-node-docker-1    |         at org.openqa.selenium.remote.http.jdk.JdkHttpMessages.notChunkingBodyPublisher(JdkHttpMessages.java:124)
grid-node-docker-1    |         at org.openqa.selenium.remote.http.jdk.JdkHttpMessages.createRequest(JdkHttpMessages.java:76)
grid-node-docker-1    |         at org.openqa.selenium.remote.http.jdk.JdkHttpClient.execute(JdkHttpClient.java:292)

Note:
We cannot set a payload also for this request, since when we do that, I hit an error as below

{
    "message": "starting container with non-empty request body was deprecated since API v1.22 and removed in v1.24"
}

with a 400 BAD REQUEST

I will raise a new PR for this in a few hours.

[krmahadevan]

Here's the new PR #11445

[pujagani]

Thank you for the details. I have reopened the associated GitHub issue as requested.