Acquire Java version simply #5552

jasontedor · 2016-07-18T20:03:57Z

Motivation:

The Java version is used for platform dependent logic. Yet, the logic
for acquiring the Java version requires special permissions (the runtime
permission "getClassLoader") that some downstream projects will never
grant. As such, these projects are doomed to have Netty act is their
Java major version is six. While there are ways to maintain the same
logic without requiring these special permissions, the logic is
needlessly complicated because it relies on loading classes that exist
in version n but not version n - 1. This complexity can be removed. As a
bonanza, the dangerous permission is no longer required.

Modifications:

Rather than attempting to load classes that exist in version n but not
in version n - 1, we can just parse the Java specification version. This
only requires a begign property (property permission
"java.specification.version") and is simple.

Result:

Acquisition of the Java version is safe and simple.

jasontedor · 2016-07-18T21:31:18Z

common/src/main/java/io/netty/util/internal/PlatformDependent.java

This is code that will execute exactly once when the class initializer for PlatformDependent is executed, I do not think we need to be concerned with that here. If the concern is genuine, there are workarounds but do note that for all practical values we are boxing to cached instances anyway (per the language specification).

I pushed fb3087ce60f4b6281401f6e8f38191fe6d68aaee to so that @netkins can simmer down. 😇

normanmaurer · 2016-07-19T04:46:12Z

@jasontedor LGTM... thanks!

johnou · 2016-07-19T06:38:29Z

common/src/main/java/io/netty/util/internal/PlatformDependent.java

And java 9? >= 8?

@johnou Thanks for looking. After Project Verona, JDK 9 can only return "9_", it's only early release builds of JDK 9 before Project Verona was integrated that can return "1.9_". No one should be using those builds any longer 😄. Yet, the method used here works on both "1.9_" and "9_" (see the unit tests added in this PR). However, your comment made me realize that this canonicalization is not needed here at all. I introduced similar code in Elasticsearch where the canonicalization is needed because it's used there to compare versions. For parsing, we don't need this. I will push a commit to remove.

jasontedor · 2016-07-19T10:40:08Z

LGTM... thanks!

@normanmaurer Thanks for the quick look. Would now be a good time to squash?

Motivation: The Java version is used for platform dependent logic. Yet, the logic for acquiring the Java version requires special permissions (the runtime permission "getClassLoader") that some downstream projects will never grant. As such, these projects are doomed to have Netty act is their Java major version is six. While there are ways to maintain the same logic without requiring these special permissions, the logic is needlessly complicated because it relies on loading classes that exist in version n but not version n - 1. This complexity can be removed. As a bonanza, the dangerous permission is no longer required. Modifications: Rather than attempting to load classes that exist in version n but not in version n - 1, we can just parse the Java specification version. This only requires a begign property (property permission "java.specification.version") and is simple. Result: Acquisition of the Java version is safe and simple.

normanmaurer · 2016-07-19T11:13:51Z

common/src/main/java/io/netty/util/internal/PlatformDependent.java

@jasontedor I just wonder if we need to guard against null ?

@normanmaurer I do not think so, as it appears to me that java.specification.version is required by the Java API specification?

Good point...

normanmaurer · 2016-07-19T11:38:26Z

@jasontedor please squash

jasontedor · 2016-07-19T11:40:04Z

@normanmaurer I squashed all commits in 03f7003.

johnou · 2016-07-19T11:45:56Z

Lgtm nice clean up!

normanmaurer · 2016-07-19T14:15:21Z

@jasontedor will merge once CI job finish

normanmaurer · 2016-07-19T16:57:09Z

Cherry-picked into 4.1 (e00b797) and 4.0 (42de9d4)

@jasontedor thanks!

jasontedor · 2016-07-19T17:38:33Z

Thanks @normanmaurer!

Scottmitch · 2016-07-20T22:48:43Z

common/src/main/java/io/netty/util/internal/PlatformDependent.java

+                }
+            });
+            return majorVersion(javaSpecVersion);
+        } catch (SecurityException e) {


should we just catch a Throwable here? (what if the string parsing fails)

In general, catching and swallowing the top-level Throwable is something that I think should be avoided. This means also catching Error for which there's generally no expectation that there is a clean recovery (e.g., OutOfMemoryError). This is especially the case for a foundational library like Netty; catching and swallowing Throwable hides these unrecoverable Errors from the application layer leaving them running on JVMs that are in a questionable state after such Errors.

It also hides bugs. In this case, string parsing can only fail if there is a bug in the parsing code or someone is running on a JVM that violates the java.specification.version format:

Specification version numbers use a Dewey decimal notation consisting of numbers seperated by periods.

And from JEP-223:

A version number, $VNUM, is a non-empty sequence of elements separated by period characters (U+002E). An element is either zero, or an unsigned integer numeral without leading zeros. The final element in a version number must not be zero. The format is:

^[1-9][0-9]*(((\.0)*\.[1-9][0-9]*)*)*$

Neither of these should be hidden, they should bubble up with the fury of a lava bubble so that it can be addressed (by fixing the broken code, or moving to a non-broken JVM).

In elastic/elasticsearch#19231, we've gone to great lengths to eliminate catching Throwable from the codebase (to set the stage for elastic/elasticsearch#19272), and if I had my druthers I'd remove it from every library that Elasticsearch depends on because we are still susceptible to swallowed OutOfMemoryError and other such Errors.

if we think these types of errors should prevent the application from starting then that is fine. I'm not sure enabling an "optimization" justifies this behavior (it may ... but I'm on the fence). I have also found it difficult in the past to catch exceptions thrown from a static context (this may have been bcz it was from Native code or for other reasons like stack trace was not populated) ... this makes the context an application developer can provide limited and makes locating these issues difficult.

if we think these types of errors should prevent the application from starting then that is fine.

Absolutely they should; again, the risks are either leaving the JVM in a questionable state or hiding bugs. No one benefits from that.

I have also found it difficult in the past to catch exceptions thrown from a static context (this may have been bcz it was from Native code or for other reasons like stack trace was not populated) ... this makes the context an application developer can provide limited and makes locating these issues difficult.

Uncaught exception handlers can help here.

jasontedor force-pushed the sane-java-version branch 3 times, most recently from 39c2269 to d884938 Compare July 18, 2016 20:11

jasontedor reviewed Jul 18, 2016
View reviewed changes

normanmaurer self-assigned this Jul 19, 2016

normanmaurer added this to the 4.0.40.Final milestone Jul 19, 2016

normanmaurer added defect improvement labels Jul 19, 2016

johnou reviewed Jul 19, 2016
View reviewed changes

normanmaurer reviewed Jul 19, 2016
View reviewed changes

jasontedor force-pushed the sane-java-version branch from bd43ebd to 03f7003 Compare July 19, 2016 11:39

normanmaurer closed this Jul 19, 2016

jasontedor deleted the sane-java-version branch July 19, 2016 17:38

Scottmitch reviewed Jul 20, 2016
View reviewed changes

Uh oh!

Acquire Java version simply #5552

Acquire Java version simply #5552

Uh oh!

Conversation

jasontedor commented Jul 18, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

normanmaurer commented Jul 19, 2016

Uh oh!

johnou Jul 19, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasontedor commented Jul 19, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasontedor Jul 19, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

normanmaurer commented Jul 19, 2016

Uh oh!

jasontedor commented Jul 19, 2016

Uh oh!

johnou commented Jul 19, 2016

Uh oh!

normanmaurer commented Jul 19, 2016

Uh oh!

normanmaurer commented Jul 19, 2016

Uh oh!

jasontedor commented Jul 19, 2016

Uh oh!

Scottmitch Jul 20, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jasontedor commented Jul 18, 2016 •

edited

Loading

johnou Jul 19, 2016 •

edited

Loading

jasontedor Jul 19, 2016 •

edited

Loading

Scottmitch Jul 20, 2016 •

edited

Loading