Illegal access not detected at compile time

[DaveCTurner]

The fundamental problem that led to #73514 is, I believe, that the compiler assumes that things will all be in the same classloader at runtime, so doesn't need to do anything special to access protected fields from one module in another module where the package names match. This wasn't a valid assumption in this case, we had matching package names in modules in different classloaders at runtime which led to the IllegalAccessException.

We've fixed this one case manually but I believe we should take action to avoid all problems of this nature. A couple of naive ideas:

• Can we inform the compiler that these modules won't be in the same classloader somehow?
• Can we forbid package name clashes across classloaders? Maybe even across modules? We duplicate package names to good effect in tests, but maybe we can be stricter about production code.

[elasticmachine]

Pinging @elastic/es-delivery (Team:Delivery)

[mark-vieira]

cc @rjernst here because this is part of the larger discussion of us slowing moving to leverage JPMS.

That said, I don't think the solution here should try to make the compilation environment mimic the runtime environment (we try to, but it'll never be a perfect solution). There are far too many gotchas there. Especially with plugins, we do all sorts of classloader/securitymanager magic that might cause stuff to compile fine but explode at runtime. In practice this stuff should be surfaced in tests. Was there some limitation or bug in our test execution that caused this issue not to be detected or was it simply a case of missing coverage?

[mark-vieira]

Also, to more directly answer your questions:

Can we inform the compiler that these modules won't be in the same classloader somehow?

Not without migrating to modules. Right now we just pass a normal classpath to javac so everything there is fair game.

Can we forbid package name clashes across classloaders? Maybe even across modules? We duplicate package names to good effect in tests, but maybe we can be stricter about production code.

As mentioned, this is an ongoing discussion but very non-trivial. We have conflicting packages all over the place that would make it impossible to just "make everything a module". This is going to be a very long-term refactor.

[DaveCTurner]

Was there some limitation or bug in our test execution that caused this issue not to be detected or was it simply a case of missing coverage?

Until today we had no tests that performed (a) a point-in-time search on (b) a frozen index with (c) the classloaders set up like they are in production. Removing any one of those three things avoids the problem, and we had tests for each pair, just not for the three things all together.

This bug apparently came about after a refactoring that simply moved some code between modules, which is the kind of thing that I expect to be able to lean on the compiler to verify. We're a bit worried that there might be other cases like this lurking in the corners of the codebase that are hard to reach in REST tests.

[DaveCTurner]

We have conflicting packages all over the place that would make it impossible to just "make everything a module". This is going to be a very long-term refactor.

Can we introduce a ratchet to stop things getting worse and help surface progress towards the goal? Conflicting package names across modules isn't something I pay much attention to (until today at least) but it would be if precommit told me off about it.

[henningandersen]

Java thinks using a protected field across class loaders in a nested class in a subclass is a violation of the spec:

https://bugs.java.com/bugdatabase/view_bug.do?bug_id=6258289

I am not sure this is affirmative, but I would suggest that we simply move towards not allowing a module to depend on another module where they share packages in production code.

Also notice that in this case the problem is most likely not caused by moving the class, rather has lied latent in the code until it was used for point-in-time here.

[mark-vieira]

Can we introduce a ratchet to stop things getting worse and help surface progress towards the goal? Conflicting package names across modules isn't something I pay much attention to (until today at least) but it would be if precommit told me off about it.

We certainly could. @rjernst recently did some analysis here on how prevalent this is. There are, frankly, a lot of them, so if we wanted to implement such a check in the near term we'd have to build in some kind of "ignore" mechanism, which then begs the question on how that get's implemented. If we simply ignore packages then we still risk folks adding more classes to those packages. Alternatively we could introduce some annotation or similar to ignore individual classes. Thoughts?

[rjernst]

I like the idea of working this down through the build. Detecting it is indeed tricky. It requires collecting global information (all package names across all our jars) and then alerting on that info.

Perhaps we could have a similar ignore list to forbidden apis missing classes? IMO an annotation would be cumbersome, and unclear where it would go (since it is the package and not individual classes). I think it would be important, though, for the list to be strict, so that if a package no longer is split, we can get an error. This will ensure everything in the list is always actionable, and any cleanups will directly remove the package name from the list (in every project that the split package existed).

There is one question about whether this should apply to tests. We can continue to run tests on the class path even in a modular world. Perhaps we could start with just the production jars that get shipped with ES?

[mark-vieira]

IMO an annotation would be cumbersome, and unclear where it would go (since it is the package and not individual classes).

The purpose of that being that we could ignore existing classes that live in clashing packages but blow up if you add a new one.

There is one question about whether this should apply to tests. We can continue to run tests on the class path even in a modular world. Perhaps we could start with just the production jars that get shipped with ES?

Agreed. And there are tangible reasons to do this with tests like having visibility into package-private code for unit testing.