[GR-49816] Throw exception for null pointers passed to RuntimeJNIAccess / RuntimeReflection register methods

[zakkak]

Don't allow null values to be passed to the register method of RuntimeJNIAccess and RuntimeReflection. Since these are public APIs GraalVM should either handle null values (by ignoring them in this case) or throw a NullPointerException before creating an asynchronous task to perform the registration in the analysis, which then results in NullPointerExceptions being thrown later when it's no longer possible to understand where the null value originate from.

Closes: quarkusio/quarkus#34698

If accepted, it would also be good to get this backported to 23.1 and 23.0 (and 22.3?).

[fniephaus]

Since these are public APIs GraalVM should either handle null values (by ignoring them in this case) or throw a NullPointerException before creating an asynchronous task to perform the registration in the analysis, which then results in NullPointerExceptions being thrown later when it's no longer possible to understand where the null value originated from.

+1 for throwing NPEs, so that users are forced to look into registrations and other API calls that do nothing (e.g., when a class/field/method lookup returned null).

[zakkak]

@fniephaus this one slept through the cracks. I have updated it to thrown NullPointerExceptions as discussed. Please have a look.

[fniephaus]

LGTM

[fniephaus]

Could you please fix the style gate?

[zakkak]

Could you please fix the style gate?

Done

[vjovanov]

Thank you greatly for this fix. LGTM.

[fniephaus]

I discussed this internally and changed the PR as follows:

• Lower the null-check into register(ConfigurationCondition condition, boolean unsafeAllocated, Class<?> clazz). It was missing there.
• Move null-checks out of runnables and right to the beginning of the register methods.
• Apply the same strategy to JNIAccessFeature. It seems you, @zakkak, forgot about that.

[zakkak]

I discussed this internally and changed the PR as follows:

Sounds good, are you going to create a different PR or force push on this?

* Lower the null-check into `register(ConfigurationCondition condition, boolean unsafeAllocated, Class<?> clazz)`. It was missing there.

That makes sense, this way it won't cover only the public facing APIs.

* Move null-checks out of runnables and right to the beginning of the register methods.

Doesn't this result in iterating over the arrays twice? Once for the check and another for the runnables?

* Apply the same strategy to `JNIAccessFeature`. It seems you, @zakkak, forgot about that.

Indeed. Thanks for noticing and fixing this.

[fniephaus]

Doesn't this result in iterating over the arrays twice? Once for the check and another for the runnables?

Yes, but there are no guarantees when the runnables run. What if they run later on in some other thread?

[zakkak]

Doesn't this result in iterating over the arrays twice? Once for the check and another for the runnables?

Yes, but there are no guarantees when the runnables run. What if they run later on in some other thread?

This is only a concern because we are using Streams instead of iterators (for loop), e.g. here, right?

Given that the arrays being iterated are not expected to be that big, what's the benefit of using streams?

[fniephaus]

Sounds good, are you going to create a different PR or force push on this?

I just pushed my changes manually.

This is only a concern because we are using Streams instead of iterators (for loop), e.g. here, right?
Given that the arrays being iterated are not expected to be that big, what's the benefit of using streams?

I'm not using streams for the null-checks. I didn't touch the other code you references.

[zakkak]

I just pushed my changes manually.

Looks good to me. Thank you Fabio.

Well runnables are inevitable in registerConditionalConfiguration either way. Sorry for the noise.

[fniephaus]

Well runnables are inevitable in registerConditionalConfiguration either way. Sorry for the noise.

What do you mean? For reachability handlers, some runnables may never run (if something did not get reachable). We should still fail if null values were registered, so that users know. In case the runnables to run later on, there is no way for users to understand which part of their code base registered a null class/method/fields, that's why we should fail as early as possible instead.

[zakkak]

Well runnables are inevitable in registerConditionalConfiguration either way. Sorry for the noise.

What do you mean? For reachability handlers, some runnables may never run (if something did not get reachable). We should still fail if null values were registered, so that users know. In case the runnables to run later on, there is no way for users to understand which part of their code base registered a null class/method/fields, that's why we should fail as early as possible instead.

Yes, I agree. Sorry if it was not clear.