Require Java 11

[titusfortner]

I'm sure there's a lot more that can be cleaned up that I'm missing, but I think this is the first step.

Description

• Requires Java 11
• Removes Netty Http client
• Defaults to Native Java

Motivation and Context

Fixes #11526

[codecov-commenter]

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (b9bdff1) 56.51% compared to head (ac44568) 56.51%.
Report is 2 commits behind head on trunk.

❗ Current head ac44568 differs from pull request most recent head 44273c7. Consider uploading reports for the commit 44273c7 to get more accurate results

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@           Coverage Diff           @@
##            trunk   #12843   +/-   ##
=======================================
  Coverage   56.51%   56.51%           
=======================================
  Files          86       86           
  Lines        5255     5255           
  Branches      187      187           
=======================================
  Hits         2970     2970           
  Misses       2098     2098           
  Partials      187      187           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[joerg1985]

This test is using the NettyClient directly, this should be fixed:

selenium/java/test/org/openqa/selenium/remote/http/RetryRequestTest.java

Line 50 in 0b5cf2d

client = new NettyClient.Factory().createClient(config);

Does it make sense to have a bazel file in the jdk folder or could this just be part of the parent "../remote/http"?

[titusfortner]

Does it make sense to have a bazel file in the jdk folder

I don't know? I mostly copied what I saw Netty had done in the places that seemed to make sense. If you think we should change something, we probably should because I don't really know what I'm changing. 😆

[joerg1985]

Does it make sense to have a bazel file in the jdk folder

I don't know? I mostly copied what I saw Netty had done in the places that seemed to make sense. If you think we should change something, we probably should because I don't really know what I'm changing. 😆

I am no bazel expert too, there might be a reason for it :D

[joerg1985]

I did some changes to make the bazel build work, i hope this is okay for you

[shs96c]

Does it make sense to have a bazel file in the jdk folder or could this just be part of the parent "../remote/http"?

The "bazel way" is a build file per java package, so the jdk folder should definitely have its own BUILD.bazel file.

[shs96c]

The NettyDomainSocketClient needs to stay, as it's used for the Docker integration because the other HttpClient implementations can't use unix domain sockets.

[joerg1985]

I had to remove the htmlunit-driver dependency to get rid of the asynchttpclient, this added a circular dependency what should be avoided. Beside some helper classes i could not find any use of the htmlunit-driver so i hope this is fine. Lets wait what the CI will tell us.

[titusfortner]

the other HttpClient implementations can't use unix domain sockets

Looks like we can transition those things when we require Java 17 next year: https://openjdk.org/jeps/380

[titusfortner]

@rbri is htmlunit-driver also moving to Java 11 requirement for the net release? Will that address the circular dependencies?

[diemol]

I don't understand why we need to make so many changes in a PR for this. We have always mentioned that moving completely to Java 11 is an iteration process.

Why don't we change the default http client, target Java 11 and that's it. The rest can be iterated little by little.

Long PRs are just too hard to review.

[joerg1985]

@rbri is htmlunit-driver also moving to Java 11 requirement for the net release? Will that address the circular dependencies?

The circular dependency is now gone due to removal of the htmlunit-driver dependency.

[joerg1985]

@diemol you are right about large PRs, but i think this PR is now ready. The CI is failing but this would also happen if only the default client has been changed. There is only one additional change here, the removal of the old netty client and it's dependencies (by removing the htmlunit-driver support).

[diemol]

However, almost all CI workflows in Java are failing. We cannot merge like that.

[titusfortner]

Yeah, I didn't want to keep distributing a library with a vulnerability, which is why I removed the netty client instead of just changing the default.

Thanks @joerg1985 for doing all the work to clean up after me. 😁

I'll take a look at the failures tomorrow

[joerg1985]

Okay then there should be single PRs created for all of the changes.

[pujagani]

This also impacts the changes done as part of #11606. Hence the failing "DevToolsReuseTest" test. Earlier, this was not a problem since we used one instance of AsyncHttpClient and it was only closed when the JVM shuts down. With the JdkHttpClient, we use a new instance as required and close it too. Changes in #11345, which assigns the client to null is while closing causing the test failure and preventing re-use. It was done to prevent memory leaks. I am not sure about the right approach to address it. We might need to discuss/brainstorm over it.

[diemol]

This also impacts the changes done as part of #11606. Hence the failing "DevToolsReuseTest" test. Earlier, this was not a problem since we used one instance of AsyncHttpClient and it was only closed when the JVM shuts down. With the JdkHttpClient, we use a new instance as required and close it too. Changes in #11345, which assigns the client to null is while closing causing the test failure and preventing re-use. It was done to prevent memory leaks. I am not sure about the right approach to address it. We might need to discuss/brainstorm over it.

@pujagani is there a bug report for that? I can imagine that someone might bump into that situation when using the Java 11 client.

[titusfortner]

Ok, the NettyDomainSocketTest calls NettyClient.Factory() which pretty much needs all of Netty plus AsyncHttpClient.

So I think we need a different test if we need a test for it.

[pujagani]

@diemol I don't think there is one. Will double check else I will create it and link it here.

[titusfortner]

This is passing all the tests and looks right to me.

The only issue is that I removed NettyDomainSocketTest because it required AHC. If we need a test for that class, can someone please help add it?

[pujagani]

#12882 - Created an issue to track the regression

[pujagani]

This is passing all the tests and looks right to me.

The only issue is that I removed NettyDomainSocketTest because it required AHC. If we need a test for that class, can someone please help add it?

I looked into it, but I don't think the current test can be modified without having another client library. Since Java HTTP Client does not support UNIX sockets.

[rbri]

@rbri is htmlunit-driver also moving to Java 11 requirement for the net release? Will that address the circular dependencies?

The circular dependency is now gone due to removal of the htmlunit-driver dependency.

@joerg1985
first of all thanks for removing the outdated stuff. I think getting rid of the circular dependency is a step forward for selenium (see my comment on #12849).

Regarding the htmlunit-driver i think i have no chance - i have to move to Java 11. I like to stay with the support of the latest selenium version.
For HtmlUnit itself i like to stay at java8 if possible but i will see.
From my point of view we can go ahead with this making a compatible htmlunit-driver is a task on my desk like all the other htmlunit related stuff (of course any help is welcome....)

[titusfortner]

Since Java HTTP Client does not support UNIX sockets.

But we're testing with java 17 and I think Java 17 does support it? Can we create an implementation in test code only?

[pujagani]

Since Java HTTP Client does not support UNIX sockets.

But we're testing with java 17 and I think Java 17 does support it? Can we create an implementation in test code only?

Ahh yes! I think that can be done. I am not sure how can we create an implementation in test code only but will be happy to take a took.