March 2017 - core-libs-dev

A couple of questions on 8160954: (spec) Runtime.Version regex and $PRE/$OPT issues
by Pavel Rappo 27 Mar '17

27 Mar '17

[This is not an RFR] I’ve been investigating JDK-8160954 [1] which mentions two separate problems within the specification of the Runtime.Version class. The first is about boundary matchers (^ and $) in the regular expression that defines the version number ($VNUM): ^[1-9][0-9]*(((\.0)*\.[1-9][0-9]*)*)*$ The reporter claims a difference between this regex and the actual behaviour of Version.parse method, namely that: Version.parse("1.1.1" + System.lineSeparator()); (*) throws an exception even though it should not. I've repeated the exercise and could not confirm the reporter's claim. Here, Version.parse method's behavior is consistent with the regex in $VNUM: System.out.println( ("1.1.1" + System.lineSeparator()) .matches("^[1-9][0-9]*(((\\.0)*\\.[1-9][0-9]*)*)*$") ); outputs: false And my reading of javadoc for java.util.regex.Pattern explains why exactly. However, I would suggest we still tweak the the $VNUM definition. The reason is that the regular expression used in $VNUM is more advanced than is actually required to specify the $VNUM. Maybe we can state it in prose a bit clearer: /** * A representation of a version string for an implementation of the - * Java SE Platform. A version string contains a version number + * Java SE Platform. A version string consists of a version number * optionally followed by pre-release and build information. * * <h2><a name="verNum">Version numbers</a></h2> @@ -960,7 +960,7 @@ * </p> * * <blockquote><pre> - * ^[1-9][0-9]*(((\.0)*\.[1-9][0-9]*)*)*$ + * [1-9][0-9]*(((\.0)*\.[1-9][0-9]*)*)* * </pre></blockquote> * * <p> The sequence may be of arbitrary length but the first three My reading is that any of the components ($VNUM, $PRE, $BUILD and $OPT) of the version string are not meant to span across multiple lines. Thus, specifying boundary matchers (^ and $) inside a pattern for a single $VNUM component looks a bit odd. If there's any place in the spec that could possibly benefit from these matchers it is the definition of $VSTR: $VNUM(-$PRE)?(\+($BUILD)?(-$OPT)?)? But even there it's pretty clear that it's the whole input that is matched against this regex. The second problem is about the optional build number ($BUILD) and the '+' separator as defined in the format for the version string ($VSTR): $VNUM(-$PRE)?(\+($BUILD)?(-$OPT)?)? The reporter claims both cases Version.parse("1.1.1-ea+--abc"); Version.parse("1.1.1-ea+"); throw IllegalArgumentException even though the $VSTR permits them. Once again I've repeated the exercise and this time I *could* confirm the behaviour. From the quasi-regex above it is expected that '+' separator might appear without the build number that follows it. And it makes a perfect sense. Otherwise a version string, say, "1.1.1-pqr" would have been ambiguous. What is "pqr" exactly? Is it an $OPT or a $PRE component? So I guess it probably means the "regex" for $VSTR is correct and the implementation of parse method is not. However, if that is the case, I don't particularly like how Version.toString behaves: /** * Returns a string representation of this version. * * @return The version string */ @Override public String toString() { StringBuilder sb = new StringBuilder(version.stream() .map(Object::toString) .collect(Collectors.joining("."))); pre.ifPresent(v -> sb.append("-").append(v)); if (build.isPresent()) { sb.append("+").append(build.get()); if (optional.isPresent()) sb.append("-").append(optional.get()); } else { if (optional.isPresent()) { sb.append(pre.isPresent() ? "-" : "+-"); (**) sb.append(optional.get()); } } return sb.toString(); } (**) means that if $PRE and $OPT are present but $BUILD is absent, then toString will produce something like this: 1.1.1-pqr-stu which will not be a valid version string according to $VSTR. And in this case we should either clearly state that toString's output is not something that can be parsed back as a version string (which would be super odd), or fix toString. I would appreciate any comments on this. Thanks, -Pavel -------------------------------------------------------------------------------- [1] https://bugs.openjdk.java.net/browse/JDK-8160954

2 1

RFR(10)(XS) 8177015: STACK_SIZE_MINIMUM of 32k is not always enough for Mac OS X
by Chris Plummer 27 Mar '17

27 Mar '17

Hello, Please review changes for the following: http://cr.openjdk.java.net/~cjplummer/8177015/webrev.00/ https://bugs.openjdk.java.net/browse/JDK-8177015 The CR description explains the problem. I'm increasing the minimum allowed -Xss stack size from 32k to 64k. There are also a couple of tests that were testing for -Xss32k that I updated to 64k. I also added a new test case for 253k. This is something I intended to do for JDK-8176768, but forgot to include the change in the review. It's not really related to this CR, but I figured I'd drop it in since I'm making other changes to the file. thanks, Chris

3 6

RFR of JDK-8176865: overridden api has a wrong since value in java.base module
by Hamlin Li 27 Mar '17

27 Mar '17

Would you please review the below patch? bug: https://bugs.openjdk.java.net/browse/JDK-8176865 webrev: http://cr.openjdk.java.net/~mli/8176865/webrev.00/ Thank you -Hamlin

2 1

RFR (JAXP) 8176405 : Catalog circular reference check did not work in certain scenarios
by huizhe wang 23 Mar '17

23 Mar '17

Hi, Please review a fix to the circular reference check that incorrectly treated duplicate entries as circular. JBS: https://bugs.openjdk.java.net/browse/JDK-8176405 Webrev: http://cr.openjdk.java.net/~joehw/jdk9/8176405/webrev/ Thanks, Joe

2 2

Re: RFR (JAXP) 8176405 : Catalog circular reference check did not work in certain scenarios
by huizhe wang 23 Mar '17

23 Mar '17

On 3/23/2017 11:08 AM, Roger Riggs wrote: > Hi Joe, > > - javax.xml.catalog.GroupEntry.set(catalog):183 > - rename the method to setCatalog(catalog) to be a bit more expressive. Done. > > - GroupEntry: 460:461; correct the @param for "catalogId" -> > "catalogURI" > Check all the @param catalogId -> catalogURI in the file > and there are extra blank lines between the @param tags Fixed. > > - CatalogImpl.java: line 432: the @param name "path" should be "uri" > to match the method signature Fixed, and plus a couple of methods where @param was missing. > > - CatalogMessages.properties: > Is it intentional that the OtherError JAXP09000002: Unexpected error > has the same number as FormatFailed: JAXP09000002? Fixed, JAXP09000003 as it should be. > > And should CircularReference = JAXP0901001 be JAXP09030004? It's parsing error but also a restriction imposed by the impl. I moved it to it's own category "Implementation restriction". > > - CatalogTest: Line 574: The Assert at 573 prints the expected and > actual. > Adding a reason as a third arg makes it easier to debug. > The println at 574 will only be printed if the test succeeds, so is > of little value Changed that to assertTrue(msg.contains(expectedMsgId)). It's safer, in case the test is run on a non-English system. Updated: JBS: https://bugs.openjdk.java.net/browse/JDK-8176405 Webrev: http://cr.openjdk.java.net/~joehw/jdk9/8176405/webrev/ Thanks, Joe > > Regards, Roger > On 3/17/2017 1:22 PM, huizhe wang wrote: > Hi, > > Please review a fix to the circular reference check that incorrectly > treated duplicate entries as circular. > > JBS: https://bugs.openjdk.java.net/browse/JDK-8176405 > Webrev: http://cr.openjdk.java.net/~joehw/jdk9/8176405/webrev/ > > Thanks, > Joe >

2 1

[8u-dev] (XS) RFR 8169056: StringIndexOutOfBoundsException in Pattern.compile with CANON_EQ flag
by Ivan Gerasimov 23 Mar '17

23 Mar '17

Hello! This is an 8u-dev only fix. (In 9 the bug was fixed with a massive update [1]). The fix is essentially one-liner: just check that the index remains within the bounds after update. Regression test was updated to cover the case. Would you please help review at your convenience? BUGURL: https://bugs.openjdk.java.net/browse/JDK-8169056 WEBREV: http://cr.openjdk.java.net/~igerasim/8169056/00/webrev/ [1] http://hg.openjdk.java.net/jdk9/dev/jdk/rev/d0c319c32334 -- With kind regards, Ivan Gerasimov

3 3

[9] RFR: 8177136: Caller sensitive method System.getLogger should specify what happens if there is no caller on the stack.
by Daniel Fuchs 23 Mar '17

23 Mar '17

Hi, Please find below an updated patch for: 8177136: Caller sensitive method System.getLogger should specify what happens if there is no caller on the stack. https://bugs.openjdk.java.net/browse/JDK-8177136 webrev: http://cr.openjdk.java.net/~dfuchs/webrev_8177136/webrev.01 System.getLogger(name) and System.getLogger(name, resourceBundle) are specified to call LoggerFinder to obtain a logger, which requires the module of the caller class. When there is no java frame on the stack, then the caller information cannot be obtained and there is not enough context to know which module would be appropriate. The proposal is to throw IllegalCallerException - which is the exception that StackWalker::getCallerClass will throw if called by System::getLogger in this case. This RFR follows the discussion started here: http://mail.openjdk.java.net/pipermail/core-libs-dev/2017-March/046868.html best regards, -- daniel

5 14

JDK 9 RFR of JDK-8177383: Wrong @modules in java/io/FilePermission/ReadFileOnPath.java
by Amy Lu 22 Mar '17

22 Mar '17

Please review the minor test-only change. @modules java.compiler was addd to tes java/io/FilePermission/ReadFileOnPath.java This should be jdk.compiler (java.compiler is just the API module, the runtime image needs to have jdk.compiler linked in to be useful) This patch also did clean up on java.compiler, if jdk.compiler is already in @modules (jdk.compiler ‘requires public java.compiler’, so java.compiler in @modules is not necessary.) bug: https://bugs.openjdk.java.net/browse/JDK-8177383 webrev: http://cr.openjdk.java.net/~amlu/8177383/webrev.00/ Thanks, Amy

2 1

RFR (JAXP) 8177350: Two missed in the change from ${java.home}/lib to ${java.home}/conf
by huizhe wang 21 Mar '17

21 Mar '17

In a jigsaw changeset [1], configuration files were moved from ${java.home}/lib directory to ${java.home}/conf. However, I found two misses as I searched the jaxp repo. javax/xml/stream/FactoryFinder.java: There were two references to the "lib" directories. The one for jaxp.properties was replaced with "conf", but the other for stax.properties was missed. javax/xml/xpath/XPathFactoryFinder.java: The jigsaw changeset contained exactly the same change as in the webrev below. It might have been missed in the source merge, the change in the jigsaw changeset was not in the JDK 9/jaxp repo. Also note that a follow-up spec change for jigsaw was documented through JDK-8049378 [2]. [1] http://hg.openjdk.java.net/jigsaw/m2/jaxp/rev/b7027b4b5c06 [2] https://bugs.openjdk.java.net/browse/JDK-8049378 JBS: https://bugs.openjdk.java.net/browse/JDK-8177350 Webrev: http://cr.openjdk.java.net/~joehw/jdk9/8177350/webrev/ Thanks, Joe

3 3

JDK 9 RFR of JDK-8177313: Move FJExceptionTableLeak.java and ConfigChanges.java back to tier1
by Amy Lu 21 Mar '17

21 Mar '17

Below two tests was demoted from tier1 to tier2 due to test fails intermittently: java/util/concurrent/forkjoin/FJExceptionTableLeak.java JDK-8144990, JDK-8151158 java/util/concurrent/ThreadPoolExecutor/ConfigChanges.java JDK-8139237, JDK-8169243 Mentioned issues have been resolved and no open bug (no failure reported) till now. This patch is to move them back to tier1. bug: https://bugs.openjdk.java.net/browse/JDK-8177313 Thanks, Amy --- old/test/TEST.groups 2017-03-21 16:07:55.000000000 +0800 +++ new/test/TEST.groups 2017-03-21 16:07:54.000000000 +0800 @@ -28,8 +28,6 @@ tier1 = \ :jdk_lang \ :jdk_util \ - -java/util/concurrent/ThreadPoolExecutor/ConfigChanges.java \ - -java/util/concurrent/forkjoin/FJExceptionTableLeak.java \ sun/nio/cs/ISO8859x.java \ java/nio/Buffer \ com/sun/crypto/provider/Cipher \ @@ -37,8 +35,6 @@ tools/pack200 tier2 = \ - java/util/concurrent/ThreadPoolExecutor/ConfigChanges.java \ - java/util/concurrent/forkjoin/FJExceptionTableLeak.java \ :jdk_io \ :jdk_nio \ -sun/nio/cs/ISO8859x.java \

3 2