Fix: don't force native access if disabled (#15815)

[cortlepp]

The current implementation of PosixNativeAccess always tries to use native access, even if this is disallowed by the JVM. This can lead to warnings on JDK24+ and will eventually be the default setting in future JVM versions. Additionally the current singleton implementation in PosixNativeAccess is problematic because it does the native setup during class initialization (<clinit>). This makes the timing of that setup unpredictable (because the JVM may load classes at any time, not just before first use) and can cause the entire application to fail if an exception is thrown (and not caught) during this setup (even if the application would work fine without native access).

This PR:

• changes the implementation to only attempt enabling native access if this is allowed by the running JVM
• changes the singleton in PosixNativeAccess to use an enum pattern

[cortlepp]

@uschindler @jpountz friendly ping because you are the main authors of this file 😄 .

[uschindler]

Hi,
thanks for the PR. In general this looks good. We use a holder pattern at different places, too - although it is strange that we really need this here, if we have the moudle check.

Can you explain why it may fail and lead to clinit problem and later ClassDefNotFoundExceptions?

There is a small change needed: Private fields should be final and therefor the one in the enum. If this is automatic (don't remember how this is in enums), I'd prefer to be explicit.

Uwe

[uschindler]

In general this can only be applied to main branch (11.0), backporting to Lucene 10 is possible, but needs a bit more adaptions:

We need to add class java.lang.Module to the stub apijar for compilation because the
isNativeAccessEnabled() enabled method is preview only, but luckily available in Java 21 and unmodified later. I can take care of this,

I forgot to add this earlier (I had it on my "brain-only-TODO-list" but forgot about it).

Can you add this to the 10.x changes list in this PR already. I will take care of the backport and add modifications to the "preview" compilation in Java 21. I will open a PR for that after we merged this.

[uschindler]

P.S.: Nice Singleton style - I like it!

[uschindler]

I still don't think that the current code fails when native access is disabled. Have you tested this?

Actually it is intended to log warning, with your change the warning is no longer printed. In general I don't see a problem with current code. If you want to get rid of the warning, suppress it in your logging config.

The current code is fine, but I don't understand why it fails. I have tested current code with disabled native access and it correctly logs a warning, so the clinit stuff is fine.

[uschindler]

Basically the current code should only be changed to have a possible UnsupportedOperationException handled also here:

https://github.com/cortlepp/lucene/blob/3ba9a0736636e976677c4c29c351945b23dfee6c/lucene/core/src/java/org/apache/lucene/store/PosixNativeAccess.java#L63-L64

So maybe move that down into the try-catch. Then it is safe for clinit. I don't see a reason for refactoring this otherwise.

Logging the warning IF native access is disabled is actually wanted.

[cortlepp]

I still don't think that the current code fails when native access is disabled. Have you tested this?

It doesn't fail, that's not what I meant. If an exception is thrown for the 2nd/3rd catch block it will fail though and then you have the described class loading issues. My main argument is that the current implementation is brittle because it can lead to wider class loading issues for a feature that is strictly speaking optional, even on posix platforms. Besides I think that it is unwise to do the setup things we need to in <clinit> in general. Class loading may be triggered by many things, it may even be loaded by accident on incompatible platforms e.g. windows (so when PosixNativeAccess should never be loaded). The new implementation fixes that, which is why I'd really prefer this or a similar pattern.

Logging the warning IF native access is disabled is actually wanted.

I kind of disagree with that, but it seems to be the general stance in Lucene so I've made my peace with it. My problem with the current implementation is that I can't suppress the warning because it always attempts the native access, which triggers a JVM warning (which I can't suppress) and then a log (which I can suppress). If I want to avoid the JVM warning I would have to add a JVM flag, which is hard in my deployment scenario where I don't have direct access to the JVM, we only provide the jar.

Can you add this to the 10.x changes list in this PR already. I will take care of the backport and add modifications to the "preview" compilation in Java 21. I will open a PR for that after we merged this.

1. do you really mean 10.x, as you mentioned in it's current form this PR only works on JDK 22+
2. As far as I understood the docs the changes.txt should only document api changes, this PR doesn't change any API (that I'm aware of)

[uschindler]

Hi,
the warning isse is the same if you don't enable vectorization preview. Lucene is written to actually "require" thos parts, but it works slower without. So we prefer to log a warning. If e.g., in Java 27, the JVM disables native access by default, we would like to avoid

Actually the current code is a bit broken due to this change by @jpountz: cortlepp@3003731

The first two lines of the static initializer must be inside the try...catch. The code is NOT briddle, sorry. The exception handling was done carefully and accoridng to the native access spec. We don't generally want to do anything like "catch (RuntimeException _)", because it would swallow an important exception. If the static initializer breaks, its a bug in the JVM and should fail.

So IMHO, the change by @jpountz should be fixed to move the first two lines of the static initializer to the try/catch block. This is indeed a bug and may fail the startup - the UnsupportedOperationException is handled there.

I can open a PR and merge this to fix this bug, but all the other code changes here are unneeded and theres no risk in the current code because all native access runtime Exceptions are handled according to the Panama Foreign spec.

[uschindler]

Actually: Your enum could also be accidentally loaded. The enum constants are also initialized on class loading so the same problems occur. So theres no reason to actually make a holder.

[uschindler]

See #15844 for the only fix needed here. I will also add a comment in source code to explain why the catch blocks are required.

I will close this issue as there is no problem requiring a refactoring. The code is not briddle and used in similar way at other static initializers.

[uschindler]

I tested all combinations in #15844. I look into adding integration test that spawns a JVM without native access. as a separate PR.

[cortlepp]

@uschindler

the warning isse is the same if you don't enable vectorization preview.

Sorry, but it really isn't because in the vectorization case I can silence the logger, in this case here my JVM will print the following:

WARNING: A restricted method in java.lang.foreign.Linker has been called
WARNING: java.lang.foreign.Linker::downcallHandle has been called by org.apache.lucene.store.PosixNativeAccess in an unnamed module (file:/usr/local/tomcat/webapps/ROOT/WEB-INF/lib/lucene-core-10.4.0.jar)
WARNING: Use --enable-native-access=ALL-UNNAMED to avoid a warning for callers in this module
WARNING: Restricted methods will be blocked in a future release unless native access is enabled

Actually: Your enum could also be accidentally loaded. The enum constants are also initialized on class loading so the same problems occur. So theres no reason to actually make a holder.

Yeah, you're right, my mistake. But IMO that means we should still change the current implementation, just not to an enum singleton holder but instead a double locking pattern (or something else that does not have the class loading issue).

[uschindler]

@uschindler

the warning isse is the same if you don't enable vectorization preview.

Sorry, but it really isn't because in the vectorization case I can silence the logger, in this case here my JVM will print the following:

WARNING: A restricted method in java.lang.foreign.Linker has been called
WARNING: java.lang.foreign.Linker::downcallHandle has been called by org.apache.lucene.store.PosixNativeAccess in an unnamed module (file:/usr/local/tomcat/webapps/ROOT/WEB-INF/lib/lucene-core-10.4.0.jar)
WARNING: Use --enable-native-access=ALL-UNNAMED to avoid a warning for callers in this module
WARNING: Restricted methods will be blocked in a future release unless native access is enabled

That's the same for Vectorization, the only difference is that it only happens when you enable Vectorization.

To get rid of the warning pass the correct command line parameter. This PR does not fix the issue. The easiest is to disallow or allow native access (see test config on other PR). It is not Lucene's problem how you configure your JVM.

Actually: Your enum could also be accidentally loaded. The enum constants are also initialized on class loading so the same problems occur. So theres no reason to actually make a holder.

Yeah, you're right, my mistake. But IMO that means we should still change the current implementation, just not to an enum singleton holder but instead a double locking pattern (or something else that does not have the class loading issue).

Why. There is no issue during class loading. If you are afraid you need to do this for every clinit anywhere in Lucene.

It is impossible that the code fails after the other PR is merged. If an exception is thrown, your JVM is broken anyways. Lucene is very strict on exceptions, e.g. we never ever catch Throwable and swallow or don't report. The patterns seen here is at many places, e.g. Panama vector init, Lucene 10 Mmap, Expressions module,... - we have many static initializers that may fail if your JVM is broken. The exception handling is always strict. We never ever intend to catch or swallow Throwable. It's always rethrowed if unknown, also in static initializers.

Sorry, Uwe

[uschindler]

In the other PR I will add some code to prevent the warning message on JVM startup.

[uschindler]

Hi,
I checked how it is implemented inside the JVM. It is impossible to prevent the warning, unless you disable native access in Lucene by default. So you can work around only by enabling or disabling it. So I can't fix this inside Lucene code:

The check for Module#isNativeAccessEnabled() always returns false when the defaults are used and it is only enabled by the code that prints the warning. So basically unless user explicitely enables native access for a specific module or allows it globally, the metadata of Module returns false, so we cant't use Module#isNativeAccessEnabled().

So actually the check you do in this PR breaks native access unless opted in, which is not what we want.

[uschindler]

Hi,
In the other PR that will be merged soon, I changed the code a bit to not fail on RuntimeException. Theoretically under some circumstances it may fail with IllegalArgException, so the hole static block handles this.
It won't catch and ignore Throwable or Errors as this is a no-go, also for static initializers.
Uwe