java.desktop module is no longer optional

[azahnen]

Search before asking

• I searched in the issues and found nothing similar.

Describe the bug

#2910/#2913 made the java.desktop module optional, which worked fine in 2.13.x.
After an upgrade to 2.15.2, starting a modular application with Jackson that does not explicitly include java.desktop will throw an error:

Exception in thread "main" java.lang.IllegalAccessError: class com.fasterxml.jackson.databind.ext.Java7SupportImpl (in module de.ii.xtraplatform.runtime) cannot access class java.beans.Transient (in module java.desktop) because module de.ii.xtraplatform.runtime does not read module java.desktop
	at de.ii.xtraplatform.runtime.intellij/com.fasterxml.jackson.databind.ext.Java7SupportImpl.<init>(Java7SupportImpl.java:22)
	at java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
	at java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:62)
	at java.base/jdk.internal.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
	at java.base/java.lang.reflect.Constructor.newInstance(Constructor.java:490)
	at de.ii.xtraplatform.runtime.intellij/com.fasterxml.jackson.databind.util.ClassUtil.createInstance(ClassUtil.java:565)
	at de.ii.xtraplatform.runtime.intellij/com.fasterxml.jackson.databind.ext.Java7Support.<clinit>(Java7Support.java:24)
	at de.ii.xtraplatform.runtime.intellij/com.fasterxml.jackson.databind.introspect.JacksonAnnotationIntrospector.<clinit>(JacksonAnnotationIntrospector.java:68)
	at de.ii.xtraplatform.runtime.intellij/com.fasterxml.jackson.databind.ObjectMapper.<clinit>(ObjectMapper.java:375)
	at de.ii.xtraplatform.runtime.intellij/io.dropwizard.jackson.Jackson.newObjectMapper(Jackson.java:45)
        at de.ii.xtraplatform.base@5.3.0-SNAPSHOT/de.ii.xtraplatform.base.domain.ConfigurationReader.<init>(ConfigurationReader.java:98)
	at de.ii.xtraplatform.base@5.3.0-SNAPSHOT/de.ii.xtraplatform.base.domain.AppLauncher.init(AppLauncher.java:109)
	at de.ii.ldproxy@3.5.0-SNAPSHOT/de.ii.xtraplatform.application.Launcher.main(Launcher.java:32)

This seems to be caused by the changes introduced in #3827. IncompatibleClassChangeError, which is considered fatal and therefore rethrown, is the super class of IllegalAccessError, which is thrown when accessing a class from an absent optional module.

Version Information

2.15.2

Reproduction

Start a modular application that does not explicitly include java.desktop and creates a new ObjectMapper.

Expected behavior

The application starts without errors.

Additional context

As a workaround, Java7Support can be initialised before creating the first ObjectMapper, which allows to catch and ignore the exception:

try{
  Java7Support java7Support = Java7Support.instance();
} catch (IllegalAccessError e) {
  // ignore
}

[JooHyukKim]

This seems might be easy to fix by either by directly modifying ExceptionUtil to not consider IllegalAccessError as fatal or by adding a conditional statement that lets IllegalAccessError pass through wherever we invoke Java7Support.

IMO, the second option may be more verbose and might only be solution to only this specific problem, but does not affect the purpose of ExceptionUtil.rethrowIfFatal.

P.S: Though I am so sure about making certain module "optional" by catching and ignoring IllegalAccessError, such implementation has been around, so maybe reasonable solution.

[pjfanning]

Catching IllegalAccessError in this special case and ignoring it seems best.

[JooHyukKim]

Until I set-up environment that can fully reproduce this issue, may I ask you to try building and running with the #4080 version, @azahnen ?

[azahnen]

@JooHyukKim Sorry, I did not find the time yet. I will test it over the weekend.

[GedMarc]

Naw this should be specified as requires static in the module file, and the module requirement should be removed only when the backwards compatible to JDK 7 is removed.

As the base version is for JDK8, clients should not be enforced to include a dependency that is only required for backwards compatibility.

The error in the description is not related to the dependency from Jackson as well, the class is used in the main module,

in module de.ii.xtraplatform.runtime) cannot access class java.beans.Transient (in module java.desktop) because module de.ii.xtraplatform.runtime does not read module java.desktop

The usage of the java.beans.x namespace in the main module is what is throwing this error is it not? In which case java.desktop must be imported into the module-info file as it is referenced directly in the code.

[cowtowncoder]

Hmmmh. To me #4080 seems to make sense, at first glance. But...

@GedMarc Moditect set up for jackson-databind already has

requires static java.desktop;

is this what you mean?
But the original stack trace would seem to suggest exception is somehow thrown by Java7SupportImpl -- would this be because something is dropping jackson-databind's module-info?

[pjfanning]

The IllegalAccessError used to be thrown and caught and ignored. I then changed the code to rethrow if the throwable was a 'fatal' error and my 'fatal' error check regards IllegalAccessError as fatal. I think #4080 is a good idea or we could remove the rethrow change that I made in this class.

[azahnen]

@JooHyukKim I tested it, works as expected. Thx!

@GedMarc The stack trace is a bit misleading since jackson is embedded in my module. But the call is definitely coming from Java7SupportImpl.

@pjfanning That is exactly how I see it.