Re: RFR 9: 8048264 : StringBuffer's codePoint methods throw unspecified IndexOutOfBoundsException
On 13/04/2015 23:49, Brent Christian wrote: Hello, Please review this small javadoc change. It was discovered that some codePoint-related methods in StringBuffer are missing documentation for throwing IndexOutOfBoundsException. The methods are: codePointAt(int) codePointBefore(int) codePointCount(int,int) offsetByCodePoints(int,int) The StringBuilder JavaDoc does have @throws tags for those methods. This looks okay. Do you have cycles to check if there are any more of these? It seems that every few years we find more cases where we have missed the inheritDoc. -Alan
Re: RFR (S): 8076461: JSR292: remove unused native and constants
Looks good. I'll push it for you. Best regards, Vladimir Ivanov On 4/14/15 2:33 PM, Michael Haupt wrote: Hi John, thanks again; I've applied your suggestions, re-tested as before and uploaded the revision to http://cr.openjdk.java.net/~mhaupt/8076461/webrev.02/. Best, Michael Am 13.04.2015 um 21:38 schrieb John Rose john.r.r...@oracle.com: That's much better; thanks. Glad to hear the verifyC's still works. The MN_* constants are a private interface between C++ and Java code. Those are the most important to verify. You can get rid of these lines; we don't look at vtable indexes any more: // The JVM uses values of -2 and above for vtable indexes. // Field values are simple positive offsets. // Ref: src/share/vm/oops/methodOop.hpp // This value is negative enough to avoid such numbers, // but not too negative. The other constants are publicly defined in various standards docs (except T_ILLEGAL). I don't think these constants are used any more, except the MN_* and REF_* ones. (The REF_* ones are in the JVM standard, so are in some sense pre-verified.) I suggest also removing the ACC_*, T_*, and CONSTANT_* names, if you can. We probably stopped using any of those when we started using ASM. Thanks! — John On Apr 13, 2015, at 4:40 AM, Michael Haupt michael.ha...@oracle.com wrote: Hi John, thank you very much for your review; keeping the Constants class around for VM/JDK constant value agreement certainly makes sense. I have undone most of the removal work and verified in a slowdebug build that MHN.verifyConstants() works. I've also added a comment on the Constants class to clarify its role a bit. Local tests and JPRT are still happy with this. Updated webrev: http://cr.openjdk.java.net/~mhaupt/8076461/webrev.01/ Best, Michael Am 07.04.2015 um 23:49 schrieb John Rose john.r.r...@oracle.com: On Apr 7, 2015, at 12:11 PM, Michael Haupt michael.ha...@oracle.com wrote: Dear all, please review and sponsor this change. Cross-posted to hs-comp and core-lib as this is at the JVM/libraries boundary. This is a straightforward refactoring change that removes many constants and unused API from MHNatives, and places some constants used only in MemberName in that class. The class MethodHandleNatives.Constants exists to enumerate and cross-check any constants which the JVM and JDK code need to agree about. Removing a constant from MethodHandleNatives.Constants (moving to MemberName) may cause failures when MHN.verifyConstants is run (via java -esa on a debug build of Java). If there are no failures, I wonder what would happen if the JVM and JDK got out of sync. in their notion of the value of a constant like MN_CALLER_SENSITIVE. It's important that some part of our release testing detect if MN_CALLER_SENSITIVE (etc.) gets out of sync. If there is some reason why this testing is no longer needed, I'd like to see the whole Constants class go away, since that's all it's really good for. But I don't see that reason yet, and moving the constants somewhere either will cause a test failure, or *should* cause a test failure. I'm happy to see the GC guys go away. They were artifacts of a quickly moving 292 implementation that spanned two repositories with unsynchronized change streams. — John RFE: https://bugs.openjdk.java.net/browse/JDK-8076461 Changes: http://cr.openjdk.java.net/~mhaupt/8076461/webrev.00/ Tested with JPRT, HotSpot testset. Thanks, Michael
Contracts, subtyping, JDK-8029804
Hi,
I've been looking into an old doc issue [1] on incompleteness in description of
handling incorrect arguments in BufferedWriter.write(String, int, int).
In a nutshell it's concerned that given the following facts:
1. Writer.write(String str, int off, int len)
promises to throw IndexOutOfBoundsException
if (off 0) or (len 0) or (off+len 0) or (off+len str.length())
2. class BufferedWriter extends Writer
3. BufferedWriter.write(String s, int off, int len)
states [2] that in contrast to its parent it won't do anything
if (len 0)
behaviour of BufferedWriter.write(String s, int off, int len) is unspecified for
other cases mentioned in its parent.
I thought it's very natural to think that you don't need to copy-paste contracts
into children unless they break it or you want to elaborate some details.
Basically because of subtype relationship, general principles of OOP and Liskov
Substitution Principle.
i.e. in this case a line in BufferedWriter.write's javadoc
* @exception IndexOutOfBoundsException {@inheritDoc}
is implied.
Unfortunately, BufferedWriter.write disobeys [3] the remainder of the contract
of its parent. So we either have to make additional notes in the javadoc like
with `len 0` case or go and fix the bug instead.
I wonder if it makes sense to fix those bugs altogether and remove the initial
`len 0` exceptional behavior. I fully understand that it formally breaks
backwards compatibility because of the cases where such calls were lurking in
code and ran unnoticed. But [2] shows that it wasn't done for some specific use
case but rather was a mistake. Thus I don't think it would be a big stretch to
assume that this thing affects no real world applications. Otherwise people
would have noticed that something is not being written at all.
P.S. I'm very interested in an outcome of this case mainly because I've noticed
some more similar things in java.io package (e.g. [4]).
Thanks.
---
[1] https://bugs.openjdk.java.net/browse/JDK-8029804
[2] https://bugs.openjdk.java.net/browse/JDK-4156573
[3] To be honest, it silently disobeys parent's guarantees, thus some
bizarre things are possible:
bw.write(abc, 1, Integer.MAX_VALUE); // ok
bw.write(abc, -1, -1); // ok
bw.write(abc, -1, 1); // undeclared StringIndexOutOfBoundsException
[4] https://bugs.openjdk.java.net/browse/JDK-8067801
Re: [9] RFR (S): Class.getSimpleName() should work for non-JLS compliant class names
David, John, thanks for review! Best regards, Vladimir Ivanov On 4/14/15 3:47 AM, David Holmes wrote: On 14/04/2015 3:39 AM, Vladimir Ivanov wrote: John, David, thanks for the feedback! What do you think about the following version: http://cr.openjdk.java.net/~vlivanov/8057919/webrev.02/jdk http://cr.openjdk.java.net/~vlivanov/8057919/webrev.02/hotspot Looks good - thanks. David Best regards, Vladimir Ivanov On 4/10/15 10:15 PM, John Rose wrote: On Apr 9, 2015, at 4:16 PM, David Holmes david.hol...@oracle.com mailto:david.hol...@oracle.com wrote: Hi Vladimir, On 10/04/2015 12:51 AM, Vladimir Ivanov wrote: David, thanks for the feedback! Updated webrev: http://cr.openjdk.java.net/~vlivanov/8057919/webrev.01/jdk http://cr.openjdk.java.net/~vlivanov/8057919/webrev.01/hotspot I restored original logic and call into VM only if it fails. This change seems fine to me, but I think John may prefer to see getSimpleName implemented such that it always returns the name from the innerclass attribute. (Though perhaps with caching if performance is a concern?) Yes, I do prefer the new logic instead of a combination of old and new. Two concerns: Somebody somewhere might be relying on a bug in the old logic and be frustrated by seeing that bug fixed. This is a fine line to walk, but in this case I think the behavior change (if any) will show up in places where we already have outages. Perhaps someone is post-processing the result of getSimpleName to correct it. If so they may no longer need to do that. Second, the new logic may itself have issues. A typical problem with the InnerClasses attribute is it is broken or stripped, or a related nestmate is missing. (See parallel thread in core-libs-dev on 8076264.) But the proposed change takes effect after a call to Class.getEnclosingClass, which uses InstanceKlass::compute_enclosing_class_impl and accesses the same InnerClasses record. This leads me to a comment: The common code (two uses of InnerClassesIterator with klass_name_at_matches) should be factored out. The factored subroutine should search out the self-record in the InnerClasses attribute. The logic is tricky enough that I'm uncomfortable with it being duplicated. Key issue: We don't want to load any classes doing this lookup. (For bonus points, factor the third use in that file, the one that looks not for self but for children-of-self. Add a boolean flag that varies the test.) Bottom line: The new logic should replace the old. The logic to compute simple name (Class.getSimpleName()) for inner/nested/local classes is tightly coupled with Java naming scheme and sometimes fails for classes generated from non-Java code. Meta-question: if this is non-Java code then what does/should simpleName even mean? Returns the simple name of the underlying class as given in the source code. If there is no (java) source code does this have any meaning? Should it return ? My reading of the JVMS is that InnerClasses attribute can be freely used by non-Java compilers to store simple names for classes they generate. The current wording for inner_name_index doesn't mention Java language. True, but it does refer to source code: represents the original simple name of C, as given in the source code from which this class file was compiled. which seems misplaced as we're discussing classes for which there is no source code as such. It could be non-Java source code. In any case, the indicated component of the binary name of the class is well-defined, whether or not it occurs in source code. Note also that classes might be generated by ASM (no source code per se) but we still have a reasonable expectation of reflecting on them, even though reflection is (mostly) defined in terms of source code constructs. Anyway it just flags to me that perhaps these Class methods need to be generalized a bit given the support for non-Java languages on the JVM. But that's for core-libs folk to decide. Yes, I think that's the issue. The multi-language folks (like me) would be disappointed to hear that we decided that non-Java languages don't have any needs here; they do. Thanks, — John Thanks, David Best regards, Vladimir Ivanov Instead of parsing class name and try to extract simple name based on JLS rules, the fix I propose is to use InnerClasses attribute from the class file. Simple name is already recorded there. JVMS-4.7.6: The InnerClasses Attribute inner_name_index: If C is anonymous (JLS §15.9.5), the value of the inner_name_index item must be zero. Otherwise, the value of the inner_name_index item must be a valid index into the constant_pool table, and the entry at that index must be a CONSTANT_Utf8_info structure (§4.4.7) that represents the original simple name of C, as given in the source code from which this class file was compiled. Since I consider backporting the fix into 8u60, I'd like to hear opinions about backward compatibility of such change. As an alternative
Re: RFR (S): 8076461: JSR292: remove unused native and constants
Hi John, thanks again; I've applied your suggestions, re-tested as before and uploaded the revision to http://cr.openjdk.java.net/~mhaupt/8076461/webrev.02/. Best, Michael Am 13.04.2015 um 21:38 schrieb John Rose john.r.r...@oracle.com: That's much better; thanks. Glad to hear the verifyC's still works. The MN_* constants are a private interface between C++ and Java code. Those are the most important to verify. You can get rid of these lines; we don't look at vtable indexes any more: // The JVM uses values of -2 and above for vtable indexes. // Field values are simple positive offsets. // Ref: src/share/vm/oops/methodOop.hpp // This value is negative enough to avoid such numbers, // but not too negative. The other constants are publicly defined in various standards docs (except T_ILLEGAL). I don't think these constants are used any more, except the MN_* and REF_* ones. (The REF_* ones are in the JVM standard, so are in some sense pre-verified.) I suggest also removing the ACC_*, T_*, and CONSTANT_* names, if you can. We probably stopped using any of those when we started using ASM. Thanks! — John On Apr 13, 2015, at 4:40 AM, Michael Haupt michael.ha...@oracle.com wrote: Hi John, thank you very much for your review; keeping the Constants class around for VM/JDK constant value agreement certainly makes sense. I have undone most of the removal work and verified in a slowdebug build that MHN.verifyConstants() works. I've also added a comment on the Constants class to clarify its role a bit. Local tests and JPRT are still happy with this. Updated webrev: http://cr.openjdk.java.net/~mhaupt/8076461/webrev.01/ Best, Michael Am 07.04.2015 um 23:49 schrieb John Rose john.r.r...@oracle.com: On Apr 7, 2015, at 12:11 PM, Michael Haupt michael.ha...@oracle.com wrote: Dear all, please review and sponsor this change. Cross-posted to hs-comp and core-lib as this is at the JVM/libraries boundary. This is a straightforward refactoring change that removes many constants and unused API from MHNatives, and places some constants used only in MemberName in that class. The class MethodHandleNatives.Constants exists to enumerate and cross-check any constants which the JVM and JDK code need to agree about. Removing a constant from MethodHandleNatives.Constants (moving to MemberName) may cause failures when MHN.verifyConstants is run (via java -esa on a debug build of Java). If there are no failures, I wonder what would happen if the JVM and JDK got out of sync. in their notion of the value of a constant like MN_CALLER_SENSITIVE. It's important that some part of our release testing detect if MN_CALLER_SENSITIVE (etc.) gets out of sync. If there is some reason why this testing is no longer needed, I'd like to see the whole Constants class go away, since that's all it's really good for. But I don't see that reason yet, and moving the constants somewhere either will cause a test failure, or *should* cause a test failure. I'm happy to see the GC guys go away. They were artifacts of a quickly moving 292 implementation that spanned two repositories with unsynchronized change streams. — John RFE: https://bugs.openjdk.java.net/browse/JDK-8076461 Changes: http://cr.openjdk.java.net/~mhaupt/8076461/webrev.00/ Tested with JPRT, HotSpot testset. Thanks, Michael -- http://www.oracle.com/ Dr. Michael Haupt | Principal Member of Technical Staff Phone: +49 331 200 7277 | Fax: +49 331 200 7561 Oracle Java Platform Group | HotSpot Compiler Team Oracle Deutschland B.V. Co. KG, Schiffbauergasse 14 | 14467 Potsdam, Germany http://www.oracle.com/commitment Oracle is committed to developing practices and products that help protect the environment
Re: RFR 8073056: Repeating annotations throws java.security.AccessControlException with a SecurityManager
Mandy, Paul, what do you think? cheers /Joel On 28 Mar 2015, at 11:24, Joel Borggrén-Franck joel.fra...@oracle.com wrote: Hi, On 27 Feb 2015, at 11:06, Peter Levart peter.lev...@gmail.com wrote: On 02/27/2015 10:27 AM, Joel Borggrén-Franck wrote: On 26 feb 2015, at 22:35, Peter Levart peter.lev...@gmail.com wrote: On 02/26/2015 10:27 PM, Peter Levart wrote: The m.setAccessible(true) for the methods is needed to access methods of non-public annotations, right? This call could be moved to AnnotationType constructor as there it will be performed only once per Method object. ...which will have the added benefit in that it will guarantee that only one MethodAccessor object per Method will ever be constructed instead of two… I don’t see this. setAccessible sets override in AccessibleObject, I don’t see a new MethodAccessor being generated here. My fault! I was mistakenly thinking of Field objects in the context of setAccessible(boolean). If you use a Field object in two modes it will create and retain two different FieldAccessors (because they are different implementations in case field is final). But I agree with you, and setting it as accessible in the AnnotationType constructor should arguably be more secure since then we know it isn’t shared since we just got our copy fresh from getDeclaredMethods(). That's a good point from maintainability perspective, yes, if someone some time decides to optimize the AnnotationType. I think it would be nice if AnnotationType documents that members() method returns Method objects that are pre-conditioned with setAccessible(true) and that users should not change this flag. I don’t want to do this in AnnotationType without a bigger overhaul that may be slightly incompatible and therefor should be 9 only. I do want to backport this fix to 8 however, so here is an alternative solution that should be safe and correct at the cost of extra allocation in the path for custom implementations of annotations (that should be rare). New webrev: http://cr.openjdk.java.net/~jfranck/8073056/webrev.02/ cheers /Joel
Re: RFR 9: 8077350 Process API Updates Implementation Review
On 04/09/2015 10:00 PM, Roger Riggs wrote:
Please review the API and implementation of the Process API Updates
described inJEP 102
https://bugs.openjdk.java.net/browse/JDK-8046092. Please review and
comment by April 23rd.
The recommendation to make ProcessHandle an interface is included
allowing the new functions to be extended by Process subclasses.
The implementation covers all functions on Unix, Windows, Solaris, and
Mac OS X.
The API doc: http://cr.openjdk.java.net/~rriggs/ph-apidraft/
The webrev: http://cr.openjdk.java.net/~rriggs/webrev-ph
Issue: JDK-8077350 https://bugs.openjdk.java.net/browse/JDK-8077350
Process API Updates Implementation
The code is in the jdk9 sandbox on branch JDK-8046092-branch.
Please review and comment, Roger
Hi Roger,
I have a few comments...
ProcessHandle JNI interface is same for all platforms and it is reused
in UNIX ProcessImpl for destroying and waiting. Is there some particular
reason why it is not reused in Windows ProcessImpl too? The Windows impl
still uses it's own native methods for destroying and waiting, but they
take process handles instead of PIDs. Are handles less susceptible to
recycling?
I am confused by the method name supportsDestroyForcibly too. The
following would be more intuitive in my opinion:
- destroyIsForcible()
- isDestroyForcible()
The @implNote for Process.onExit() says:
366 * @implNote
367 * The default implementation of this method employs a thread to
368 * {@link #waitFor() wait} for process exit,
369 * which may consume a lot of memory for thread stacks if a
370 * large number of processes are waited for concurrently.
That's true for the default implementation in Process but not true for
internal OpenJDK ProcessImpl(s). Should the note say so explicitly so
that users won't be afraid of using this method with internal
implementations?
371 * p
372 * External implementations are advised to override this
method and provide
373 * more efficient implementation. For example, to delegate to
the underlying
374 * process, it can simply do the following:
375 * pre{@code
376 *public CompletableFutureProcess onExit() {
377 * return delegate.onExit();
378 *}
379 * }/pre
380 * ...which in case of implementation of the delegate is used.
...this example is not correct as it exposes the delegate Process
instance. It should be something like:
public CompletableFutureProcess onExit() {
return delegate.onExit().thenApply(p - this);
}
Otherwise it's nice how this turned out.
Unrelated to your implementation, I have been thinking of another small
Process API update. Some people find it odd how redirected in/out/err
streams are exposed:
http://blog.headius.com/2013/06/the-pain-of-broken-subprocess.html
They basically don't like:
- that exposed Input/Output streams are buffered
- that underlying streams are File(Input/Output)Streams which, although
the backing OS implementation are not files but pipes, don't expose
selectable channels so that non-blocking event-based IO could be
performed on them.
- that exposed IO streams are automatically managed in UNIX variants
of ProcessImpl which needs subtle hacks to do it in a perceptively
transparent way (delayed close, draining input on exit and making it
available after the underlying handle is already closed, ...)
So I've been playing with the idea of exposing the real pipe channels
in last couple of days. Here's the prototype I came up with:
http://cr.openjdk.java.net/~plevart/jdk9-sandbox/JDK-8046092-branch/Process.PipeChannel/webrev.01/
This adds new Redirect type to the API and 3 new methods to Process that
return Pipe channels when this new Redirect type is used. It's
interesting that no native code changes were necessary. The behavior of
pipes on Windows is a little different (perhaps because the Pipe NIO API
uses sockets under the hood on Windows - why is that? Windows does have
a pipe equivalent). What bothers me is that file handles opened on files
(when redirecting to/from File) can be closed as soon as the subprocess
is started and the subprocess is still able to read/write from the files
(like with UNIX). It's not the same with pipe (i.e. socket) handles on
Windows. They must be closed only after subprocess exits.
If this subtle difference between file handles and socket handles on
Windows could be dealt with (perhaps some options exist that affect
subprocess spawning), then the extra waiting thread would not be needed
on Windows.
So what do you thik of this API update?
Regards, Peter
Re: AWT Dev [9] Review Request: 8076264 [macosx] Launching app on MacOSX requires enclosing class
Hi Sergey,
looks good to me.
Thanks
Kumar
On 4/14/2015 9:02 AM, Sergey Bylokhov wrote:
Hi, Kumar.
Thanks for your suggestions. Then new version of the fix:
http://cr.openjdk.java.net/~serb/8076264/webrev.02
On 10.04.15 4:39, Kumar Srinivasan wrote:
src/java.base/macosx/native/libjli/java_md_macosx.c
we need a comment here explaining why the Exception is cleared,
basically what
you have explained below.
test/tools/launcher/MainClassWithoutEnclosingClass.java
:) the test seems to be more complicated than the source changes,
the launcher tests have had their own test framework TestHelper, to
keep these tests consistent, I have enclosed a simpler test
implementation,
I have not tested this on MacOSX as I don't have one, however I have
tested
on Windows, I trust you will verify the test, with JPRT on all
platforms.
Many thanks for looking into this!.
Kumar
---CUT
/*
* Copyright (c) 2015, Oracle and/or its affiliates. All rights
reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
* under the terms of the GNU General Public License version 2 only, as
* published by the Free Software Foundation.
*
* This code is distributed in the hope that it will be useful, but
WITHOUT
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
* version 2 for more details (a copy is included in the LICENSE file
that
* accompanied this code).
*
* You should have received a copy of the GNU General Public License
version
* 2 along with this work; if not, write to the Free Software
Foundation,
* Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
*
* Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA
94065 USA
* or visit www.oracle.com if you need additional information or have
any
* questions.
*/
/**
* @test
* @bug 8076264
* @summary Launching app shouldn't require enclosing class for the
main class.
* @compile TestMainWithoutEnclosing.java
* @run main TestMainWithoutEnclosing
*/
import java.io.File;
import java.io.IOException;
import java.util.ArrayList;
import java.util.List;
public class TestMainWithoutEnclosing extends TestHelper {
static final String EnclosingName = Enclosing;
static void createJarFile(File testJar) throws IOException {
ListString scratch = new ArrayList();
scratch.add(public class Enclosing {);
scratch.add(public static final class Main {);
scratch.add(public static void main(String... args)
{);
scratch.add(System.out.println(\Hello World\););
scratch.add(});
scratch.add(});
scratch.add(});
File enclosingFile = new File(EnclosingName + .java);
createFile(enclosingFile, scratch);
compile(enclosingFile.getName());
// avoid side effects remove the Enclosing class
getClassFile(enclosingFile).delete();
createJar(cvfe, testJar.getName(), EnclosingName + $Main,
EnclosingName + $Main + .class);
// remove extraneous files in the current directory
new File(EnclosingName + $Main + .class).delete();
}
public static void main(String... args) throws IOException {
File testJarFile = new File(test.jar);
createJarFile(testJarFile);
TestResult tr = doExec(javaCmd, -jar, testJarFile.getName());
if (!tr.isOK()) {
System.out.println(tr);
throw new RuntimeException(test returned non-positive
value);
}
if (!tr.contains(Hello World)) {
System.out.println(tr);
throw new RuntimeException(expected output not found);
}
}
}
---CUT
On 4/9/2015 3:06 PM, Sergey Bylokhov wrote:
Hello,
Can somebody from the core team take a look?
Thanks.
On 08.04.15 16:29, Sergey Bylokhov wrote:
Hello.
Please review the fix for jdk 9.
After the fix we clear all errors caused by getCanonicalName() on
the mainclass and leave JAVA_MAIN_CLASS_ empty. This empty case
will be handled in the NSApplicationAWT.m.
Plus small cleanup suggested in the bug report(jstring-jclass).
Bug: https://bugs.openjdk.java.net/browse/JDK-8076264
Webrev can be found at:
http://cr.openjdk.java.net/~serb/8076264/webrev.01
Re: AWT Dev [9] Review Request: 8076264 [macosx] Launching app on MacOSX requires enclosing class
Hi, Kumar.
Thanks for your suggestions. Then new version of the fix:
http://cr.openjdk.java.net/~serb/8076264/webrev.02
On 10.04.15 4:39, Kumar Srinivasan wrote:
src/java.base/macosx/native/libjli/java_md_macosx.c
we need a comment here explaining why the Exception is cleared,
basically what
you have explained below.
test/tools/launcher/MainClassWithoutEnclosingClass.java
:) the test seems to be more complicated than the source changes,
the launcher tests have had their own test framework TestHelper, to
keep these tests consistent, I have enclosed a simpler test
implementation,
I have not tested this on MacOSX as I don't have one, however I have
tested
on Windows, I trust you will verify the test, with JPRT on all
platforms.
Many thanks for looking into this!.
Kumar
---CUT
/*
* Copyright (c) 2015, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
* under the terms of the GNU General Public License version 2 only, as
* published by the Free Software Foundation.
*
* This code is distributed in the hope that it will be useful, but
WITHOUT
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
* version 2 for more details (a copy is included in the LICENSE file
that
* accompanied this code).
*
* You should have received a copy of the GNU General Public License
version
* 2 along with this work; if not, write to the Free Software Foundation,
* Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
*
* Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065
USA
* or visit www.oracle.com if you need additional information or have any
* questions.
*/
/**
* @test
* @bug 8076264
* @summary Launching app shouldn't require enclosing class for the
main class.
* @compile TestMainWithoutEnclosing.java
* @run main TestMainWithoutEnclosing
*/
import java.io.File;
import java.io.IOException;
import java.util.ArrayList;
import java.util.List;
public class TestMainWithoutEnclosing extends TestHelper {
static final String EnclosingName = Enclosing;
static void createJarFile(File testJar) throws IOException {
ListString scratch = new ArrayList();
scratch.add(public class Enclosing {);
scratch.add(public static final class Main {);
scratch.add(public static void main(String... args) {);
scratch.add(System.out.println(\Hello World\););
scratch.add(});
scratch.add(});
scratch.add(});
File enclosingFile = new File(EnclosingName + .java);
createFile(enclosingFile, scratch);
compile(enclosingFile.getName());
// avoid side effects remove the Enclosing class
getClassFile(enclosingFile).delete();
createJar(cvfe, testJar.getName(), EnclosingName + $Main,
EnclosingName + $Main + .class);
// remove extraneous files in the current directory
new File(EnclosingName + $Main + .class).delete();
}
public static void main(String... args) throws IOException {
File testJarFile = new File(test.jar);
createJarFile(testJarFile);
TestResult tr = doExec(javaCmd, -jar, testJarFile.getName());
if (!tr.isOK()) {
System.out.println(tr);
throw new RuntimeException(test returned non-positive
value);
}
if (!tr.contains(Hello World)) {
System.out.println(tr);
throw new RuntimeException(expected output not found);
}
}
}
---CUT
On 4/9/2015 3:06 PM, Sergey Bylokhov wrote:
Hello,
Can somebody from the core team take a look?
Thanks.
On 08.04.15 16:29, Sergey Bylokhov wrote:
Hello.
Please review the fix for jdk 9.
After the fix we clear all errors caused by getCanonicalName() on
the mainclass and leave JAVA_MAIN_CLASS_ empty. This empty case will
be handled in the NSApplicationAWT.m.
Plus small cleanup suggested in the bug report(jstring-jclass).
Bug: https://bugs.openjdk.java.net/browse/JDK-8076264
Webrev can be found at:
http://cr.openjdk.java.net/~serb/8076264/webrev.01
--
Best regards, Sergey.
Re: [9] RFR of 8075156: (prefs) remove() should disallow the use of the null control character '\u0000' as key
Hi Roger, Thanks for taking the time to review this recap. Your suggestion to forge ahead with the approach already taken seems to be reasonable. I concur that a fail-fast approach of throwing an IAE as soon as the bad character is encountered if preferable to waiting for some inscrutable subsequent exception in flush() or sync(). So barring objections to the contrary from some other quarter, I will clean up the current patch and also address the errors in the class level javadoc that I pointed out in AbstractPreferences. Thanks, Brian On Apr 14, 2015, at 8:00 AM, Roger Riggs roger.ri...@oracle.com wrote: Thanks for digging deeper and the recap. I don't see any cases in which it is necessary or valuable to allow \0 in Strings (key or value). The original bug report did not indicate whether it was discovered as a testing exercise or when diagnosing a bug in an application. The compatibility risk is unknown at the moment since no cases have been noted that require \0 in strings. Long term the code will be easier to maintain if it is less complex and has fewer variables that are platform or format specific and developer errors are detected as soon as possible. Of the possible remedies below, #2 seems the most practical. But it will further hide the problem from developers since calling flush and sync is never required for correct operation: ///explicit //flush//invocation is //not//required upon termination to ensure that pending updates are made persistent/ Flush and sync would be the only opportunity to throw an exception and explain the cause. It will be harder to track down since the true cause will be far removed from the time/location of the exception. In the absence of advice to continue to support \0in Preference strings on Windows or Mac I'd continue with the recent direction to make \0 in key and value strings throw IAE.
Re: [9] RFR of 8075156: (prefs) remove() should disallow the use of the null control character '\u0000' as key
Hi Brian, Thanks for digging deeper and the recap. I don't see any cases in which it is necessary or valuable to allow \0 in Strings (key or value). The original bug report did not indicate whether it was discovered as a testing exercise or when diagnosing a bug in an application. The compatibility risk is unknown at the moment since no cases have been noted that require \0 in strings. Long term the code will be easier to maintain if it is less complex and has fewer variables that are platform or format specific and developer errors are detected as soon as possible. Of the possible remedies below, #2 seems the most practical. But it will further hide the problem from developers since calling flush and sync is never required for correct operation: ///explicit //flush//invocation is //not//required upon termination to ensure that pending updates are made persistent/ Flush and sync would be the only opportunity to throw an exception and explain the cause. It will be harder to track down since the true cause will be far removed from the time/location of the exception. In the absence of advice to continue to support \0in Preference strings on Windows or Mac I'd continue with the recent direction to make \0 in key and value strings throw IAE. Roger On 4/10/2015 6:26 PM, Brian Burkhalter wrote: On Apr 4, 2015, at 12:53 PM, Alan Bateman alan.bate...@oracle.com wrote: On 24/03/2015 19:20, Brian Burkhalter wrote: Please review at your convenience. Issue: https://bugs.openjdk.java.net/browse/JDK-8075156 Patch: http://cr.openjdk.java.net/~bpb/8075156/webrev.00/ This is a sequel to the resolved issue https://bugs.openjdk.java.net/browse/JDK-8068373, (prefs) FileSystemPreferences writes \0 to XML storage, causing loss of all preferences, wherein the code point U+, the null control character, was made illegal to use as a key in the generic Unix file system-based Preferences. The issue at hand extends disallowing U+ as a key in the put() method on Mac OS X and Windows, and also disallows this use to the remove() methods on these platforms and in the generic Unix file system-based Preferences. Use of U+ in the corresponding get() method has not been disallowed as this method returns a default value. If it would be preferable that the behavior of get() with respect to the key U+ were the same as for put() and remove() then this patch may be updated to that effect. Minimally then the putXXX methods should make it clear that they throw IAE for this case. This would be a javadoc only change because the implementation does this as a consequence of the original patch. Agreed. Actually I am not completely satisfied with the fix for https://bugs.openjdk.java.net/browse/JDK-8068373 so I went back over all the discussions and notes on the various ways to fix the problem trying to rethink whether there might be a better solution. The problem is not really with the Preferences APIs per se, but rather with the ability of the XML-based backing store to handle all characters which might be present in a String, in this particular case \u, but there are other possible problematic characters as well. Ideally, the fix would be to modify the XML backing store to handle all possible characters. This does not however appear to be possible without introducing an incompatibility when the XML backing store is shared between the set of newer versions of Java which would support storing all characters and the older versions which do not. There are ways to minimize the incompatibility but not apparently eliminate it and the additional complexity might not merit the effort. In the RFC thread on 8068373, it was concluded that it would be better simply to disallow \u for XML-backed Preferences. This change however introduced an incompatibility with non-XML backing stores which might or might not be able to handle \u. So to address this incompatibility (and to address the companion get/remove methods) the present RFR was introduced. The present change has the potential however to break Preferences implementations which have a backing store for which \u *is* legal. Note also that all Preferences implementations should be able to handle all Strings if there is no interaction with the backing store, even in the case of the XML backing store. Without a round trip via the XML backing store no error would be encountered. This suggests an alternative fix of allowing the illegal character to be used “live” but disallowed from being written to a backing store which cannot handle it. I raise these alternatives here because if any were preferable, then there is no point in going further in the current direction as one changeset reversion would already be needed. In the original discussion then it was just a question as to whether get/getXXX and remove should be consistent. If the get and remove methods will always behave as if the key doesn't exist
Re: Contracts, subtyping, JDK-8029804
Hi Pavel,
On 14/04/2015 9:57 PM, Pavel Rappo wrote:
Hi,
I've been looking into an old doc issue [1] on incompleteness in description of
handling incorrect arguments in BufferedWriter.write(String, int, int).
In a nutshell it's concerned that given the following facts:
1. Writer.write(String str, int off, int len)
promises to throw IndexOutOfBoundsException
if (off 0) or (len 0) or (off+len 0) or (off+len str.length())
2. class BufferedWriter extends Writer
3. BufferedWriter.write(String s, int off, int len)
states [2] that in contrast to its parent it won't do anything
if (len 0)
behaviour of BufferedWriter.write(String s, int off, int len) is unspecified for
other cases mentioned in its parent.
I thought it's very natural to think that you don't need to copy-paste contracts
into children unless they break it or you want to elaborate some details.
Basically because of subtype relationship, general principles of OOP and Liskov
Substitution Principle.
i.e. in this case a line in BufferedWriter.write's javadoc
* @exception IndexOutOfBoundsException {@inheritDoc}
is implied.
@throws rather than @exception but yes, unfortunately historically
people have been unaware of the need to explicitly state in the javadoc
that a subclass inherits the unchecked-exceptions of its parent. This is
usually an oversight, but of course a subtype is allowed to weaken
preconditions.
Unfortunately, BufferedWriter.write disobeys [3] the remainder of the contract
of its parent. So we either have to make additional notes in the javadoc like
with `len 0` case or go and fix the bug instead.
Yep. In this case it seems odd to allow the parents preconditions to be
weakened but I can't say whether someone thought there was a valid
reason for it. But once the implementation is out of sync with the
implied/assumed contract then you are looking at a spec change
regardless - either to make the inherited spec clear (and fix the
implementation) or to clearly document the change in behaviour.
I wonder if it makes sense to fix those bugs altogether and remove the initial
`len 0` exceptional behavior. I fully understand that it formally breaks
backwards compatibility because of the cases where such calls were lurking in
code and ran unnoticed. But [2] shows that it wasn't done for some specific use
case but rather was a mistake. Thus I don't think it would be a big stretch to
assume that this thing affects no real world applications. Otherwise people
would have noticed that something is not being written at all.
Code that writes nothing when passed a negative 'len' is probably
exactly what the user expects. If an exception were originally thrown
then to me it seems likely the user would have added the if (len 0)
check themselves and so still write nothing in this case. So no bug
would have been observed. If you start throwing the exception now then
existing code may be broken.
When the code has been out of sync with the spec for an extended period
of time the safe approach is to adapt the spec to the code, not the
other way around.
David
P.S. I'm very interested in an outcome of this case mainly because I've noticed
some more similar things in java.io package (e.g. [4]).
Thanks.
---
[1] https://bugs.openjdk.java.net/browse/JDK-8029804
[2] https://bugs.openjdk.java.net/browse/JDK-4156573
[3] To be honest, it silently disobeys parent's guarantees, thus some
bizarre things are possible:
bw.write(abc, 1, Integer.MAX_VALUE); // ok
bw.write(abc, -1, -1); // ok
bw.write(abc, -1, 1); // undeclared StringIndexOutOfBoundsException
[4] https://bugs.openjdk.java.net/browse/JDK-8067801
Re: RFR 9: 8077350 Process API Updates Implementation Review
Hi Peter,
Thanks for the review.
On 4/14/2015 11:47 AM, Peter Levart wrote:
On 04/09/2015 10:00 PM, Roger Riggs wrote:
Please review the API and implementation of the Process API Updates
described inJEP 102
https://bugs.openjdk.java.net/browse/JDK-8046092. Please review and
comment by April 23rd.
The recommendation to make ProcessHandle an interface is included
allowing the new functions to be extended by Process subclasses.
The implementation covers all functions on Unix, Windows, Solaris,
and Mac OS X.
The API doc: http://cr.openjdk.java.net/~rriggs/ph-apidraft/
The webrev: http://cr.openjdk.java.net/~rriggs/webrev-ph
Issue: JDK-8077350 https://bugs.openjdk.java.net/browse/JDK-8077350
Process API Updates Implementation
The code is in the jdk9 sandbox on branch JDK-8046092-branch.
Please review and comment, Roger
Hi Roger,
I have a few comments...
ProcessHandle JNI interface is same for all platforms and it is reused
in UNIX ProcessImpl for destroying and waiting. Is there some
particular reason why it is not reused in Windows ProcessImpl too? The
Windows impl still uses it's own native methods for destroying and
waiting, but they take process handles instead of PIDs. Are handles
less susceptible to recycling?
I am confused by the method name supportsDestroyForcibly too. The
following would be more intuitive in my opinion:
- destroyIsForcible()
- isDestroyForcible()
We're tried several variations including the word 'destroy' and it still
seems confusing.
I suggested the supportsNormalTermination name in another thread.
The @implNote for Process.onExit() says:
366 * @implNote
367 * The default implementation of this method employs a thread to
368 * {@link #waitFor() wait} for process exit,
369 * which may consume a lot of memory for thread stacks if a
370 * large number of processes are waited for concurrently.
That's true for the default implementation in Process but not true for
internal OpenJDK ProcessImpl(s). Should the note say so explicitly so
that users won't be afraid of using this method with internal
implementations?
Yes, it should be more specific about ProcessBuilder created instances.
371 * p
372 * External implementations are advised to override this
method and provide
373 * more efficient implementation. For example, to delegate to
the underlying
374 * process, it can simply do the following:
375 * pre{@code
376 *public CompletableFutureProcess onExit() {
377 * return delegate.onExit();
378 *}
379 * }/pre
380 * ...which in case of implementation of the delegate is used.
...this example is not correct as it exposes the delegate Process
instance. It should be something like:
public CompletableFutureProcess onExit() {
return delegate.onExit().thenApply(p - this);
}
Will fix tomorrow.
Otherwise it's nice how this turned out.
Unrelated to your implementation, I have been thinking of another
small Process API update. Some people find it odd how redirected
in/out/err streams are exposed:
http://blog.headius.com/2013/06/the-pain-of-broken-subprocess.html
yep, I've read that several times.
They basically don't like:
- that exposed Input/Output streams are buffered
- that underlying streams are File(Input/Output)Streams which,
although the backing OS implementation are not files but pipes, don't
expose selectable channels so that non-blocking event-based IO could
be performed on them.
- that exposed IO streams are automatically managed in UNIX variants
of ProcessImpl which needs subtle hacks to do it in a perceptively
transparent way (delayed close, draining input on exit and making it
available after the underlying handle is already closed, ...)
So I've been playing with the idea of exposing the real pipe
channels in last couple of days. Here's the prototype I came up with:
http://cr.openjdk.java.net/~plevart/jdk9-sandbox/JDK-8046092-branch/Process.PipeChannel/webrev.01/
This adds new Redirect type to the API and 3 new methods to Process
that return Pipe channels when this new Redirect type is used. It's
interesting that no native code changes were necessary. The behavior
of pipes on Windows is a little different (perhaps because the Pipe
NIO API uses sockets under the hood on Windows - why is that? Windows
does have a pipe equivalent). What bothers me is that file handles
opened on files (when redirecting to/from File) can be closed as soon
as the subprocess is started and the subprocess is still able to
read/write from the files (like with UNIX). It's not the same with
pipe (i.e. socket) handles on Windows. They must be closed only after
subprocess exits.
If this subtle difference between file handles and socket handles on
Windows could be dealt with (perhaps some options exist that affect
subprocess spawning), then the extra waiting thread would not be
needed on Windows.
So what do you think of
Re: RFR 9: 8077350 Process API Updates Implementation Review
On 14/04/2015 16:47, Peter Levart wrote: The behavior of pipes on Windows is a little different (perhaps because the Pipe NIO API uses sockets under the hood on Windows - why is that? Windows does have a pipe equivalent). Multiplexing is a bit limited on Windows, I don't think you can select on both sockets and pipe handlers in the one select. It should be rare to need to do this of course. So it should be possible to have the Windows Selector partition the selectable channels so that they are multiplexed via different threads when it arises. -Alan
Re: [9] RFR of 8075156: (prefs) remove() should disallow the use of the null control character '\u0000' as key
Please review at your convenience this latest patch modified from the previous one pursuant to the most recent comments. Issue: https://bugs.openjdk.java.net/browse/JDK-8075156 Patch: http://cr.openjdk.java.net/~bpb/8075156/webrev.01/ Summary: * Revise Preferences javadoc to indicate IAE for put*(), get*(), and remove() for NUL control character U+ in the key and / or value, as appropriate. * Extend fix of put*() in JDK-8068373 to all platforms by moving functional code up to to AbstractPreferences. * Add fix for get*() and remove() in AbstractPreferences. * Revert FileSystemPreferences changes from JDK-8068373 as these are now handled by AbstractPreferences (note this includes rolling back the more recent year in the copyright as well). * Minor picayune cleanup in WindowsPreferences. * Correct class javadoc of AbstractPreferences which had referred to flush() and sync() as returning boolean type. * Extend existing test to get() and remove() cases. Test passed on all the usual platforms. The CCC request will need to be revised or a new one filed, pending approval of this patch. Thanks, Brian On Apr 14, 2015, at 8:37 AM, Brian Burkhalter brian.burkhal...@oracle.com wrote: So barring objections to the contrary from some other quarter, I will clean up the current patch and also address the errors in the class level javadoc that I pointed out in AbstractPreferences.
Re: JDK-8060068/JDK-8067904 DriverManager clean-up introduces bootstrap problem that breaks production drivers
Hi Robert,
I had a chance to look at the code you provided and part of the problem is that
MyDriver is not adhering to the JDBC specification as the call to
registerDriver *must* be in the static block of the java.sql.Driver
implementation and *not* in the constructor. The registerWithDriverManager()
logic and the fact the call to this method is in the constructor is the primary
culprit.
I will probably put back the static block at least in the short term but please
work with the vendor to address their issue
Best,
Lance
On Apr 3, 2015, at 5:33 AM, Robert Gibson robbiexgib...@yahoo.com wrote:
Hi there,
We are doing some early testing with JDK 9 and have discovered that the
changes made to java.sql.DriverManager back in November/December have
introduced an incompatibility with our JDBC driver (that we have used
unchanged since Java 6) when it is pre-loaded with Class.forName (as
recommended by Tomcat and specified as being harmless in the JavaDoc). We
filed a bug report but it got an incident ID JI-9019539 and then disappeared
into a black hole.
In the hope of the problem getting a bit more attention I attach a
reproducible test case that is representative of the commercially-available
JDBC driver that we use (hint: the name rhymes with MyFace and it's used in a
lot of banks). I guess the problem is that the driver is calling
DriverManager.getDrivers() as part of its registration process, which is now
causing the stack overflow given at the end of this mail, after the code.
If you compile the attached files into test.jar and run
java -cp test.jar JDBCTest n
java -cp test.jar JDBCTest y
then the test should print
Trying to connect to jdbc:mydriver
OK
in both cases (as it does with Java 8).
When running with Java 9, the second command prints
Trying to connect to jdbc:mydriver
FAIL
No suitable driver found for jdbc:mydriver
Hope this is helpful.
Regards,
Robert
JDBCTest.java
import java.io.*;
import java.sql.*;
import java.util.*;
public class JDBCTest
{
public static void main(String... argv) throws Exception
{
if (argv.length 1) DriverManager.setLogWriter(new
PrintWriter(System.out));
String[] urls = new String[] {jdbc:mydriver};
if (argv.length 0 argv[0].equals(y)) {
loadClass(MyDriver);
}
for (String url : urls) {
System.out.println(Trying to connect to + url);
try {
Connection c = DriverManager.getConnection (url);
if (c == null) System.out.println(FAIL);
else System.out.println(OK);
}catch(SQLException e) {
System.out.println(FAIL);
System.out.println(e.getMessage());
}
}
}
public static void loadClass(String classname) {
try { Class.forName(classname); } catch(ClassNotFoundException e) {
System.out.println(Class not found: + classname); }
}
}
MyDriver.java
import java.sql.*;
import java.util.Enumeration;
import java.util.Properties;
import java.util.logging.Logger;
public class MyDriver implements java.sql.Driver {
static {
new MyDriver();
}
public MyDriver() {
this.registerWithDriverManager();
}
public final Connection connect(String url, Properties props) throws
SQLException {
return acceptsURL(url) ? new MyConnection() : null;
}
public boolean acceptsURL(String url) throws SQLException {
return url.startsWith(jdbc:mydriver);
}
public DriverPropertyInfo[] getPropertyInfo(String var1, Properties props)
throws SQLException {
return null;
}
public int getMajorVersion() {
return 0;
}
public int getMinorVersion() {
return 1;
}
public boolean jdbcCompliant() {
return false;
}
public Logger getParentLogger() throws SQLFeatureNotSupportedException {
throw new SQLFeatureNotSupportedException(No logging for you);
}
protected void registerWithDriverManager() {
try {
synchronized(DriverManager.class) {
DriverManager.registerDriver(this);
Enumeration e = DriverManager.getDrivers();
while(e.hasMoreElements()) {
Driver d = (Driver)e.nextElement();
if(d instanceof MyDriver d != this) {
DriverManager.deregisterDriver(d);
}
}
}
} catch (SQLException ex) {
}
}
}
MyConnection.java
import java.sql.*;
import java.util.Map;
import java.util.Properties;
import java.util.concurrent.Executor;
public class MyConnection implements Connection {
@Override
public Statement createStatement() throws SQLException {
return null;
}
@Override
public PreparedStatement
