jai-imageio / jai-imageio-core

JAI ImageIO Core (without javax.media.jai dependencies)
Other
234 stars 87 forks source link

Java 9 java.specification.version failure #24

Closed simonsteiner1984 closed 6 years ago

simonsteiner1984 commented 8 years ago

Java 9 has java.specification.version == 9 ImageIO.getReaderMIMETypes();

Caused by: java.lang.NumberFormatException: For input string: "" at java.lang.NumberFormatException.forInputString(java.base@9-ea/NumberFormatException.java:65) at java.lang.Integer.parseInt(java.base@9-ea/Integer.java:705) at java.lang.Integer.parseInt(java.base@9-ea/Integer.java:813) at com.github.jaiimageio.impl.common.ImageUtil.processOnRegistration(ImageUtil.java:1401)

tballison commented 8 years ago

Just ran into this one, too, when testing Apache Tika with 9-ea.

THausherr commented 8 years ago

https://bugs.openjdk.java.net/browse/JDK-8154860

jpsacha commented 8 years ago

The problem is with the code in ImageUtil line 1388:

String jvmVersionString = 
    System.getProperty("java.specification.version");
int verIndex = jvmVersionString.indexOf("1.");
// Skip the "1." part to get to the part of the version number that
// actually changes from version to version
// The assumption here is that "java.specification.version" is 
// always of the format "x.y" and not "x.y.z" since that is what has
// been practically observed in all JDKs to-date, an examination of
// the code returning this property bears this out. However this does
// not guarantee that the format won't change in the future, 
// though that seems unlikely.
jvmVersionString = jvmVersionString.substring(verIndex+2);

int jvmVersion = Integer.parseInt(jvmVersionString);

For Java 9 (current build) the "java.specification.version" has value just "9". The above code attempts to skip "1." at the beginning without checking that it is there.

A simple workaround would be to do a basic check:

String jvmVersionString = 
    System.getProperty("java.specification.version");
// Skip "1." in version name
if(jvmVersionString.contains("1.")) {
    // Now the old way, assuming that version always starts with "1.", true prior to version 9

    int verIndex = jvmVersionString.indexOf("1.");
    // Skip the "1." part to get to the part of the version number that
    // actually changes from version to version
    // The assumption here is that "java.specification.version" is
    // always of the format "x.y" and not "x.y.z" since that is what has
    // been practically observed in all JDKs to-date, an examination of
    // the code returning this property bears this out. However this does
    // not guarantee that the format won't change in the future,
    // though that seems unlikely.
    jvmVersionString = jvmVersionString.substring(verIndex + 2);
}

int jvmVersion = Integer.parseInt(jvmVersionString);
THausherr commented 8 years ago

Re version strings, you may want to read the discussion here: https://issues.apache.org/jira/browse/PDFBOX-3155

Btw the JDK bug has been "solved", now it displays a stack trace and no longer fails.

don-vip commented 7 years ago

Can you please apply the patch proposed by @jpsacha ? Java 9 will be released in two months.

sfeigl commented 6 years ago

Any chance we get a release that is JDK 9 fixed soon?

nipafx commented 6 years ago

I might open a PR soon. In preparation I asked on StackOverflow for a good way to determine the major version.

simonsteiner1984 commented 6 years ago

They have example code in https://issues.apache.org/jira/browse/PDFBOX-3155

christopher-johnson commented 6 years ago

The version finding method as the accepted answer in StackOverflow should replace the method in ImageUtil. @nicolaiparlog please submit a PR, so that we can resolve this very old (and annoying) issue. Thank you.

nipafx commented 6 years ago

@christopher-johnson I'd rather wait for a maintainer signaling readiness to accept such a PR.

lfcnassif commented 6 years ago

Any updates here?

mipastgt commented 6 years ago

Hi Nicolai, could you please submit your PR. This bug is really a pain for everybody using Java 9 and beyond and 10 is just arround the corner. I doubt that you will get an explicit invitation here :-)

pejobo commented 6 years ago

@stain Hi Stian, this looks like a deadlock situation:

nicolaiparlog commented on 11 Jan

@christopher-johnson I'd rather wait for a maintainer signaling readiness to accept such a PR.

stain commented 6 years ago

Hi, sorry about the hold up @pejobo - I've invited you as a maintainer and merged #48.

I can do the Maven release next week. Is there anything else that needs to go in?

stain commented 6 years ago

Invited @pejobo and @nicolaiparlog as maintainers.

nipafx commented 6 years ago

Err, wow, that's quite the field promotion, thank you.

48 solved this by string parsing - is there any interest in a solution like the one in the SO answer?

pejobo commented 6 years ago

Using reflection could induce other problems, I think the current solution is "good enough".

nipafx commented 6 years ago

Just in case you worry about the module system intervening and making reflection problematic, I want to point out that that's not a risk. All the involved classes and members are accessible and could be called just as well by non-reflection code. The only reason I don't do that is because it would make it necessary to compile the file with Java 9, which complicates things a lot.

That aside, I agree - the current solution is good enough. :+1:

stain commented 6 years ago

Reflection can also be blocked by security policies. A "clean" solution would be an intermediate class that delegates to a second class that calls Runtime.version normally, with a second fall-back to the current method if that intermediate class failed to load (because Runtime.version lacks before Java9). But yes, that would require that intermediate class to be compiled on Java 9.

lbellonda commented 6 years ago

Hello, I think that solution simpler is better. I would like a solution that can be compiled on previous JDK without problems. The current solution is good.

It is not possible check only first digits in the version string (just in case of a future 9.5) or trap the excution anyway in a catch block? The failure to detect the version at the moment is not a real problem given where it is used in the code (jvmVendor.equals("Sun Microsystems Inc.")).

At the moment the version is calculated outside of the block of code that use it. Moving the calculation inside the "if" would mitigate the problem.

Checking only digits allows the code to be binary compatible with some small changes in the JDK string format without forcing a new publication in a Maven repository.

christopher-johnson commented 6 years ago

A point of reference is org.gradle.api.JavaVersion

Where

println System.getProperty("java.specification.version")
println System.getProperty("java.version")
println Jvm.current().getJavaVersion()

produces: 9 9.0.4 1.9

Note that the JavaVersion is returned from getJavaVersion() in a backwards compatible format, despite the New JDK Versioning Scheme. The newly merged method is simple and will work, though a JavaVersion class that produces a stable string from java.version seems a bit more robust.

lbellonda commented 6 years ago

Hello, the test from which the problem is born is for Java vendor "java.vendor" equals to "Sun Microsystems Inc.". Should the test be updated for Oracle too?

pejobo commented 6 years ago

Should the test be updated for Oracle too?

The "Oracle" vs. "Sun" thing is a bigger code change, where more places in the code are affected. I would suggest to open a new bug and push a release with the latest code now.