-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Ensure that the extended socket options TCP_KEEPXXX are available #88935
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
Conversation
|
Can we drop the use of reflection in (FWIW #88929 removes the mention of JDKs before JDK11 from the relevant docs) |
|
Pinging @elastic/es-distributed (Team:Distributed) |
|
Hi @ChrisHegarty, I've created a changelog YAML for you. |
Yes. I wanted to keep the changes as small as possible, but let me see how the refactoring looks. |
I'd also prefer that. At a minimum we should change instead, since this was where the error originated. But we might as well go all in here. Happy to help out on this. |
|
The use of reflection has been removed. There is a little indirection still, through accessors to the extended socket options, so as to narrow the suppression of the forbidden non-portable ExtendedSocketOptions type. In reality, while support for extended socket options is platform and implementation specific, all downstream distributions of OpenJDK have the ExtendedSocketOptions type defined. |
henningandersen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but I'd like David or Ryan to also review.
modules/transport-netty4/src/main/java/org/elasticsearch/transport/netty4/NetUtils.java
Outdated
Show resolved
Hide resolved
modules/transport-netty4/src/test/java/org/elasticsearch/transport/netty4/NetUtilsTests.java
Outdated
Show resolved
Hide resolved
modules/transport-netty4/src/test/java/org/elasticsearch/transport/netty4/NetUtilsTests.java
Show resolved
Hide resolved
rjernst
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
distribution/tools/server-cli/src/main/java/org/elasticsearch/server/cli/ServerProcess.java
Outdated
Show resolved
Hide resolved
DaveCTurner
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM3
|
Thanks for the reviews. I'm going to merge only after a full clean CI run on all platforms. And will then proceed to backport to 8.4, and 8.3.x. |
|
@elasticmachine run elasticsearch-ci/packaging-tests-unix-sample |
|
@elasticmachine run elasticsearch-ci/part-1-windows |
|
@elasticmachine run elasticsearch-ci/packaging-tests-unix-sample |
A NIO socket channel supports setting and retrieving a set of socket
options. A socket channel supports a specified set of standard socket
options, e.g.
SO_RCVBUF, as well as optional implementation specificextended options. The network socket channels in the JDK support,
among others, the
TCP_KEEPIDLE,TCP_KEEPINTERVAL, andTCP_KEEPCOUNTextended socket options (on certain platforms). Theseextended socket options are only available when their defining module,
jdk.net, is present, and also when defined to the bootstrap classloader - the socket channel implementation in
java.baseusesits class loader (the bootstrap class loader) to indirectly initialize
jdk.net.ExtendedSocketOptions. Therefore, thejdk.netmodule must bein the same class loader - the bootstrap class loader - for the extended
socket options to be available/supported **.
The transport-netty4 component sets a number of extended socket options
on its network channels. Specifically, the
TCP_KEEPIDLE,TCP_KEEPINTERVAL, andTCP_KEEPCOUNTsocket options. The coderesponsible for this uses reflection with a silent fallback if the
options are not available, see
o.e.transport.netty4.NetUtils. The useof reflection is because of the desire to compile and run on JDK 8 when
initially added. This is no longer a requirement, since the minimal
runtime version is now JDK 17 and the extended socket options are
available since JDK 11.
With the move to Java modules, only the explicitly required modules or
modules resolved during service binding, are resolved from the JDK. The
jdk.netmodule is not currently resolved, since the extended socketoptions are not directly used by any code defined by the system class
loader. Therefore the extended socket options are not available /
supported by the NIO socket channels in the runtime.
The transport-netty4 component, and its dependencies, are defined as
modules by a module layer that is a child of the boot layer. The code in
the transport-netty4 component refers to the socket options and attempts
to set them on a socket channel. While this is all fine and as it should
be, it will not succeed unless the socket channel implementation
supports the extended socket options, which requires the
jdk.netmodule to be resolved in the boot module layer (and loaded by the
bootstrap class loader).
Finally, the solution is to simply resolve the
jdk.netmodule in theboot layer. We do this by adding the
--add-modules=jdk.netoption,rather than an explicit
requiresdirective in the server module-info,since no code in server uses the extended socket options. Additionally,
the transport-netty4 module-info has a requires directive added to
ensure that the
jdk.netmodule is present. This directive doesn'tensure that
jdk.netis in the right module layer, but should besufficient for now, and will allow future refactorings in
NetUtilstoreplace the use of reflective access to the socket options with static
access.
Note: I'm still considering if or how best a test could be added to
assert this, but really once NetUtils is refactored that should be
sufficient.
closes #88897
**