Integrated: 8263108: Class initialization deadlock in java.lang.constant
On Tue, 9 Mar 2021 13:46:04 GMT, Jaikiran Pai wrote: > Can I please get a review for this proposed patch for the issue reported in > https://bugs.openjdk.java.net/browse/JDK-8263108? > > As noted in that issue, the `java.lang.constant.DynamicConstantDesc` and > `java.lang.constant.ConstantDescs` can end up in a classloading deadlock due > to the nature of the code involved in their static blocks. A thread dump of > one such deadlock (reproduced using the code attached to that issue) is as > follows: > > "Thread A" #14 prio=5 os_prio=31 cpu=101.45ms elapsed=8.30s > tid=0x7ff88e01ca00 nid=0x6003 in Object.wait() [0x7a4c6000] >java.lang.Thread.State: RUNNABLE > at > java.lang.constant.DynamicConstantDesc.(java.base@16-ea/DynamicConstantDesc.java:67) > - waiting on the Class initialization monitor for > java.lang.constant.ConstantDescs > at Deadlock.threadA(Deadlock.java:14) > at Deadlock$$Lambda$1/0x000800c00a08.run(Unknown Source) > at java.lang.Thread.run(java.base@16-ea/Thread.java:831) > > "Thread B" #15 prio=5 os_prio=31 cpu=103.15ms elapsed=8.30s > tid=0x7ff88b936a00 nid=0x9b03 in Object.wait() [0x7a5c9000] >java.lang.Thread.State: RUNNABLE > at > java.lang.constant.ClassDesc.ofDescriptor(java.base@16-ea/ClassDesc.java:145) > - waiting on the Class initialization monitor for > java.lang.constant.DynamicConstantDesc > at > java.lang.constant.ConstantDescs.(java.base@16-ea/ConstantDescs.java:239) > at Deadlock.threadB(Deadlock.java:24) > at Deadlock$$Lambda$2/0x000800c00c28.run(Unknown Source) > at java.lang.Thread.run(java.base@16-ea/Thread.java:831) > > The commit in this PR resolves that deadlock by moving the `canonicalMap` > initialization in `DynamicConstantDesc`, from the static block, to a lazily > initialized map, into the `tryCanonicalize` (private) method of the same > class. That's the only method which uses this map. > > The implementation in `tryCanonicalize` carefully ensures that the deadlock > isn't shifted from the static block to this method, by making sure that the > `synchronization` on `DynamicConstantDesc` in this method happens only when > it's time to write the state to the shared `canonicalMap`. This has an > implication that the method local variable `canonDescs` can get initialized > by multiple threads unnecessarily but from what I can see, that's the only > way we can avoid this deadlock. This penalty of multiple threads initializing > and then throwing away that map isn't too huge because that will happen only > once when the `canonicalMap` is being initialized for the first time. > > An alternate approach that I thought of was to completely get rid of this > shared cache `canonicalMap` and instead just use method level map (that gets > initialized each time) in the `tryCanonicalize` method (thus requiring no > locks/synchronization). I ran a JMH benchmark with the current change > proposed in this PR and with the alternate approach of using the method level > map. The performance number from that run showed that the approach of using > the method level map has relatively big impact on performance (even when > there's a cache hit in that method). So I decided to not pursue that > approach. I'll include the benchmark code and the numbers in a comment in > this PR, for reference. > > The commit in this PR also includes a jtreg testcase which (consistently) > reproduces the deadlock without the fix and passes when this fix is applied. > Extra manual testing has been done to verify that the classes of interest > (noted in that testcase) are indeed getting loaded in those concurrent > threads and not in the main thread. For future maintainers, there's a > implementation note added on that testcase explaining why it cannot be moved > into testng test. This pull request has now been integrated. Changeset: 9225a230 Author:Jaikiran Pai URL: https://git.openjdk.java.net/jdk/commit/9225a230 Stats: 180 lines in 2 files changed: 169 ins; 9 del; 2 mod 8263108: Class initialization deadlock in java.lang.constant Reviewed-by: vtewari, plevart, chegar - PR: https://git.openjdk.java.net/jdk/pull/2893
Re: RFR: 8254979: Class.getSimpleName() returns non-empty for lambda and method
Hello,
On 3/17/2021 3:56 PM, jmehrens wrote:
On Tue, 16 Mar 2021 22:24:55 GMT, Joe Darcy wrote:
[snip]
780:
781: /**
782: *{@return {@code true} if and only if this class has the synthetic
modifier
The braces around return tag are allowed? "{@return ...} vs. "@return ..."
Seems inconsistent with other uses of return tags.
Yes; after a recently-added javadoc feature in JDK 16:
JDK-8075778: Add javadoc tag to avoid duplication of return
information in simple situations.
https://bugs.openjdk.java.net/browse/JDK-8075778
The usage allow an explicit @return tag to be elided when it is the same
as the first sentence of the javadoc.
The docs in the java.compiler module have been updated to use the feature.
HTH,
-Joe
Re: RFR: 8254979: Class.getSimpleName() returns non-empty for lambda and method
On Tue, 16 Mar 2021 22:24:55 GMT, Joe Darcy wrote:
> java.lang.Class has a number of methods to return some kind of textual token
> associated with the class, including a type name, canonical name, simple
> name, and separately the results of toString().
>
> Bug 8254979 notes that getSimpleName mentions returning the name in source
> code, but operationally returns a non-null result for synthetic classes, ones
> without benefit of source code representation. Since it is useful to return a
> non-null result in such cases, I've updated the spec to mention this case.
> The names of synthetic classes are not covered by the JLS; using a "$" in
> such names is customary, but not strictly required. The synthetic classes
> used to implement lambdas and method references as cited in the bug report
> include "$"'s.
>
> Looking over the other "getFooName" methods, I don't think they need to be
> updated to handle this case.
src/java.base/share/classes/java/lang/Class.java line 782:
> 780:
> 781: /**
> 782: *{@return {@code true} if and only if this class has the synthetic
> modifier
The braces around return tag are allowed? "{@return ...} vs. "@return ..."
Seems inconsistent with other uses of return tags.
-
PR: https://git.openjdk.java.net/jdk/pull/3038
Re: RFR: 8262807: Note assumptions of core reflection modeling and parameter handling [v4]
> Please review the javadoc change below, written in response to recent > discussion on core-libs. > > The bulk of the change is to add a discussion to java.lang.reflect's > package-info file about a language vs JVM model for core reflection. That > discussion is then linked to from several relevant locations core reflection. > A discussion of generic parameter handling is also added along with various > small cleanups. > > I'll update copyright, etc. after agreement on the text is reached. Joe Darcy has updated the pull request incrementally with one additional commit since the last revision: Per current terminology conventions, "types" -> "classes and interfaces" in package-info. - Changes: - all: https://git.openjdk.java.net/jdk/pull/3036/files - new: https://git.openjdk.java.net/jdk/pull/3036/files/c2bf7434..cd6fd6fe Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=3036=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3036=02-03 Stats: 18 lines in 1 file changed: 0 ins; 0 del; 18 mod Patch: https://git.openjdk.java.net/jdk/pull/3036.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3036/head:pull/3036 PR: https://git.openjdk.java.net/jdk/pull/3036
Integrated: 8262807: Note assumptions of core reflection modeling and parameter handling
On Tue, 16 Mar 2021 17:22:13 GMT, Joe Darcy wrote: > Please review the javadoc change below, written in response to recent > discussion on core-libs. > > The bulk of the change is to add a discussion to java.lang.reflect's > package-info file about a language vs JVM model for core reflection. That > discussion is then linked to from several relevant locations core reflection. > A discussion of generic parameter handling is also added along with various > small cleanups. > > I'll update copyright, etc. after agreement on the text is reached. This pull request has now been integrated. Changeset: 99b39aad Author:Joe Darcy URL: https://git.openjdk.java.net/jdk/commit/99b39aad Stats: 108 lines in 4 files changed: 69 ins; 15 del; 24 mod 8262807: Note assumptions of core reflection modeling and parameter handling Reviewed-by: rriggs - PR: https://git.openjdk.java.net/jdk/pull/3036
Re: RFR: 8262807: Note assumptions of core reflection modeling and parameter handling [v3]
> Please review the javadoc change below, written in response to recent > discussion on core-libs. > > The bulk of the change is to add a discussion to java.lang.reflect's > package-info file about a language vs JVM model for core reflection. That > discussion is then linked to from several relevant locations core reflection. > A discussion of generic parameter handling is also added along with various > small cleanups. > > I'll update copyright, etc. after agreement on the text is reached. Joe Darcy has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains five additional commits since the last revision: - Merge and update copyright year. - Merge branch 'master' into 8262807 - Respond to review feedback. - Appease jcheck. - 8262807: Note assumptions of core reflection modeling and parameter handling - Changes: - all: https://git.openjdk.java.net/jdk/pull/3036/files - new: https://git.openjdk.java.net/jdk/pull/3036/files/3f102171..c2bf7434 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=3036=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3036=01-02 Stats: 2708 lines in 204 files changed: 1275 ins; 709 del; 724 mod Patch: https://git.openjdk.java.net/jdk/pull/3036.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3036/head:pull/3036 PR: https://git.openjdk.java.net/jdk/pull/3036
Re: RFR: 8262807: Note assumptions of core reflection modeling and parameter handling [v2]
On Tue, 16 Mar 2021 20:41:24 GMT, Joe Darcy wrote: >> Please review the javadoc change below, written in response to recent >> discussion on core-libs. >> >> The bulk of the change is to add a discussion to java.lang.reflect's >> package-info file about a language vs JVM model for core reflection. That >> discussion is then linked to from several relevant locations core >> reflection. A discussion of generic parameter handling is also added along >> with various small cleanups. >> >> I'll update copyright, etc. after agreement on the text is reached. > > Joe Darcy has updated the pull request incrementally with one additional > commit since the last revision: > > Respond to review feedback. looks good. - Marked as reviewed by rriggs (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/3036
Re: RFR: 8189198: Add "forRemoval = true" to Applet API deprecations
On Wed, 17 Mar 2021 19:02:39 GMT, Alan Bateman wrote:
>
>
> > I cannot find any instances where the scope can be narrowed
>
> Are you about AquaInternalFrameUI.mouseRelased, BasicPopupMenuUI.
> stateChanged, and other non-trivial methods?
These have the code pattern such as:
} else if (c instanceof JApplet) {
putting '@SuppressWarnings("removal")' before the declaration of 'c' does not
help, because the code is not an assignment to 'c'
-
PR: https://git.openjdk.java.net/jdk/pull/2920
Integrated: 8254979: Class.getSimpleName() returns non-empty for lambda and method
On Tue, 16 Mar 2021 22:24:55 GMT, Joe Darcy wrote: > java.lang.Class has a number of methods to return some kind of textual token > associated with the class, including a type name, canonical name, simple > name, and separately the results of toString(). > > Bug 8254979 notes that getSimpleName mentions returning the name in source > code, but operationally returns a non-null result for synthetic classes, ones > without benefit of source code representation. Since it is useful to return a > non-null result in such cases, I've updated the spec to mention this case. > The names of synthetic classes are not covered by the JLS; using a "$" in > such names is customary, but not strictly required. The synthetic classes > used to implement lambdas and method references as cited in the bug report > include "$"'s. > > Looking over the other "getFooName" methods, I don't think they need to be > updated to handle this case. This pull request has now been integrated. Changeset: 26234b53 Author:Joe Darcy URL: https://git.openjdk.java.net/jdk/commit/26234b53 Stats: 10 lines in 1 file changed: 3 ins; 1 del; 6 mod 8254979: Class.getSimpleName() returns non-empty for lambda and method Reviewed-by: rriggs, mchung - PR: https://git.openjdk.java.net/jdk/pull/3038
Re: RFR: 8263729: [test] Extend time to wait before destroying child in ProcssBuilder Basic test
On Wed, 17 Mar 2021 19:52:49 GMT, Roger Riggs wrote: > Its a Java Child for consistency across tests and across OS's. > The JavaChild executes a number of specialized commands to consume or provide > data to the parent. > Piecing that together on different OS's would add more variables to the tests. > In this particular case, assuming it's well understood could probably be > handled by some native program > as long its available on all platforms consistently. Thanks Roger for explaining. Sorry for the questions. We are bugged regularly by similar intermittent errors and I am curious and interested in getting tests stable. I'm still thinking reading both stdout and stderr simultaneously would be more defensive in case the child dies of unnatural circumstances, or prints out unexpected output. The VM does this sometimes (eg unconditonal logging, or crashing), and we have seen blockages like these when childs waited on unflushed write buffers. Since you have all parts already there - a thread to drain the childs output - why not just spawn two of those instead of one and make sure both are closed as expected when the child is gone. That way you also cut down on execution time. - PR: https://git.openjdk.java.net/jdk/pull/3049
Re: RFR: 8263729: [test] Extend time to wait before destroying child in ProcssBuilder Basic test
On Wed, 17 Mar 2021 19:35:14 GMT, Thomas Stuefe wrote: >>> The child does no (zero) writes to either stream. It is invoked only to >>> sleep until it is destroyed. >>> The purpose of the test is to verify the exception that is thrown when the >>> other end(child) of the pipe is closed (because the process has been >>> forcibly terminated). >> >> I meant as a consequence of an error. E.g. a crash or a native OOM could >> cause the child to write an error report to stderr. > >> Hmm, maybe the child can create a file to indicate that bootstrap has >> finished. > > Why does the child even have to be a java VM? Its a Java Child for consistency across tests and across OS's. The JavaChild executes a number of specialized commands to consume or provide data to the parent. Piecing that together on different OS's would add more variables to the tests. In this particular case, assuming it's well understood could probably be handled by some native program as long its available on all platforms consistently. - PR: https://git.openjdk.java.net/jdk/pull/3049
Re: RFR: 8263729: [test] Extend time to wait before destroying child in ProcssBuilder Basic test
On Wed, 17 Mar 2021 19:34:19 GMT, Thomas Stuefe wrote: >> Hmm, maybe the child can create a file to indicate that bootstrap has >> finished. > >> The child does no (zero) writes to either stream. It is invoked only to >> sleep until it is destroyed. >> The purpose of the test is to verify the exception that is thrown when the >> other end(child) of the pipe is closed (because the process has been >> forcibly terminated). > > I meant as a consequence of an error. E.g. a crash or a native OOM could > cause the child to write an error report to stderr. > Hmm, maybe the child can create a file to indicate that bootstrap has > finished. Why does the child even have to be a java VM? - PR: https://git.openjdk.java.net/jdk/pull/3049
Re: RFR: 8263729: [test] Extend time to wait before destroying child in ProcssBuilder Basic test
On Wed, 17 Mar 2021 19:30:54 GMT, Ioi Lam wrote: >> The child does no (zero) writes to either stream. It is invoked only to >> sleep until it is destroyed. >> The purpose of the test is to verify the exception that is thrown when the >> other end(child) of the pipe is closed (because the process has been >> forcibly terminated). > > Hmm, maybe the child can create a file to indicate that bootstrap has > finished. > The child does no (zero) writes to either stream. It is invoked only to sleep > until it is destroyed. > The purpose of the test is to verify the exception that is thrown when the > other end(child) of the pipe is closed (because the process has been forcibly > terminated). I meant as a consequence of an error. E.g. a crash or a native OOM could cause the child to write an error report to stderr. - PR: https://git.openjdk.java.net/jdk/pull/3049
Re: RFR: 8260605: Various java.lang.invoke cleanups [v7]
On Wed, 17 Mar 2021 18:15:37 GMT, Mandy Chung wrote: >> Other methods that delegate to `makeImpl` aren't doing up-front validation, >> so this change was made to get things more in line. It might be good to >> spell out that `makeImpl` does these checks for all its callers, though. >> (The `makeImpl` fast-path that execute before the validation can never >> return an invalid MethodType) > > Generally we have a public API implementation to check the arguments upfront > for readability. In particular for this case, the validation cost is > negligible and removing the validation makes the code unclear where the > validation is done. I prefer to keep the validation there. It should check > that `nptype` is non-null and not `void.class`. Ah, I mis-read your comment. I now see that `makeImpl` does the argument validation. Adding the comment would be helpful. - PR: https://git.openjdk.java.net/jdk/pull/2300
Re: RFR: 8260605: Various java.lang.invoke cleanups [v7]
On Wed, 17 Mar 2021 19:12:10 GMT, Claes Redestad wrote: >> - Remove unused code >> - Inline and simplify the bootstrap method invocation code (remove pointless >> reboxing checks etc) >> - Apply pattern matching to make some code more readable > > Claes Redestad has updated the pull request incrementally with one additional > commit since the last revision: > > Rename className -> name, prefixedClassName -> className, makeImpl input > validation commentary Thanks for doing the renames. Looks good. - Marked as reviewed by mchung (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/2300
Re: RFR: 8263729: [test] Extend time to wait before destroying child in ProcssBuilder Basic test
On Wed, 17 Mar 2021 19:13:24 GMT, Roger Riggs wrote: >> Hmm. That's strange. >> >> One issue I don't understand with this test, should the parent not read from >> both streams simultaneously, because if the child is sending output to the >> stream the parent is not reading from the child may block on write? >> >> This depends on file io buffer size of course but if the child writes some >> larger output into e.g. stderr while the parent reads stdout, parent will >> wait on stdout and child will wait on the write to stderr finishing. So, >> either we should read both streams or redirect the stream we are not >> interested in to paren'ts stdout/stderr where hopefully the testrunner >> itself would read it. >> >> I may misunderstand the test completely; if yes, sorry for the confusion! >> >> Thomas > > The child does no (zero) writes to either stream. It is invoked only to > sleep until it is destroyed. > The purpose of the test is to verify the exception that is thrown when the > other end(child) of the pipe is closed (because the process has been forcibly > terminated). Hmm, maybe the child can create a file to indicate that bootstrap has finished. - PR: https://git.openjdk.java.net/jdk/pull/3049
Re: RFR: 8263729: [test] Extend time to wait before destroying child in ProcssBuilder Basic test
On Wed, 17 Mar 2021 17:53:14 GMT, Ioi Lam wrote: >> I don't think this is CPU starvation but memory exhaustion. _beginthreadex >> fails with EACCES if it has no resources to start the thread, which in this >> case probably means memory (the other possibility would be >> out-of-HANDLE-space but seeing that the child just started I don't see how >> this could be). >> >> Should we harden tests against resource starvation like this, or rather >> require the test machine to be beefy enough for tests? Also, I don't >> understand, if the child has not enough resources to bring the VM fully up >> how waiting on either stream would help. > > @tstuefe It's unlikely that _beginthreadex failed due to lack of memory. We > are running on a machine with more than 50GB ram with only concurrency of 6. > Extracts from the logs: > > $ jtreg -vmoption:-Xmx512m -concurrency:6 -vmoption:-XX:MaxRAMPercentage=4 > open/test/jdk:jdk_lang > > start = Wed Mar 10 22:54:53 GMT 2021 > end = Wed Mar 10 22:56:36 GMT 2021 > elapsed= 102932 0:01:42.932 > > > [2021-03-10 22:57:07] [C:\cygwin\bin\free.exe] timeout=2 > > total used free shared buff/cache available > Mem: 50068964 19974888 30094076 0 0 30094076 > Swap: 8388608 105048 8283560 > > [2021-03-10 22:57:07] exit code: 0 time: 33 ms > > > My theory is that `TerminateProcess` has made it impossible for the child > process to spawn new threads, but somehow existing threads are still able to > run a little bit and produce the log message. > > Of course this is just a theory, and I cannot find any supporting docs from > MS. However, if we implement the work around and make sure we don't kill the > child until it has finished bootstrapping, and the bug doesn't happen > anymore, then we know something more. Hmm. That's strange. One issue I don't understand with this test, should the parent not read from both streams simultaneously, because if the child is sending output to the stream the parent is not reading from the child may block on write? This depends on file io buffer size of course but if the child writes some larger output into e.g. stderr while the parent reads stdout, parent will wait on stdout and child will wait on the write to stderr finishing. So, either we should read both streams or redirect the stream we are not interested in to paren'ts stdout/stderr where hopefully the testrunner itself would read it. I may misunderstand the test completely; if yes, sorry for the confusion! Thomas - PR: https://git.openjdk.java.net/jdk/pull/3049
Re: RFR: 8263729: [test] Extend time to wait before destroying child in ProcssBuilder Basic test
On Wed, 17 Mar 2021 19:10:24 GMT, Thomas Stuefe wrote: >> @tstuefe It's unlikely that _beginthreadex failed due to lack of memory. We >> are running on a machine with more than 50GB ram with only concurrency of 6. >> Extracts from the logs: >> >> $ jtreg -vmoption:-Xmx512m -concurrency:6 -vmoption:-XX:MaxRAMPercentage=4 >> open/test/jdk:jdk_lang >> >> start = Wed Mar 10 22:54:53 GMT 2021 >> end = Wed Mar 10 22:56:36 GMT 2021 >> elapsed= 102932 0:01:42.932 >> >> >> [2021-03-10 22:57:07] [C:\cygwin\bin\free.exe] timeout=2 >> >> total used free shared buff/cache available >> Mem: 50068964 19974888 30094076 0 0 30094076 >> Swap: 8388608 105048 8283560 >> >> [2021-03-10 22:57:07] exit code: 0 time: 33 ms >> >> >> My theory is that `TerminateProcess` has made it impossible for the child >> process to spawn new threads, but somehow existing threads are still able to >> run a little bit and produce the log message. >> >> Of course this is just a theory, and I cannot find any supporting docs from >> MS. However, if we implement the work around and make sure we don't kill the >> child until it has finished bootstrapping, and the bug doesn't happen >> anymore, then we know something more. > > Hmm. That's strange. > > One issue I don't understand with this test, should the parent not read from > both streams simultaneously, because if the child is sending output to the > stream the parent is not reading from the child may block on write? > > This depends on file io buffer size of course but if the child writes some > larger output into e.g. stderr while the parent reads stdout, parent will > wait on stdout and child will wait on the write to stderr finishing. So, > either we should read both streams or redirect the stream we are not > interested in to paren'ts stdout/stderr where hopefully the testrunner itself > would read it. > > I may misunderstand the test completely; if yes, sorry for the confusion! > > Thomas The child does no (zero) writes to either stream. It is invoked only to sleep until it is destroyed. The purpose of the test is to verify the exception that is thrown when the other end(child) of the pipe is closed (because the process has been forcibly terminated). - PR: https://git.openjdk.java.net/jdk/pull/3049
Re: RFR: 8260605: Various java.lang.invoke cleanups [v7]
> - Remove unused code > - Inline and simplify the bootstrap method invocation code (remove pointless > reboxing checks etc) > - Apply pattern matching to make some code more readable Claes Redestad has updated the pull request incrementally with one additional commit since the last revision: Rename className -> name, prefixedClassName -> className, makeImpl input validation commentary - Changes: - all: https://git.openjdk.java.net/jdk/pull/2300/files - new: https://git.openjdk.java.net/jdk/pull/2300/files/62ec7a40..0f9f5efa Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=2300=06 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2300=05-06 Stats: 34 lines in 2 files changed: 5 ins; 5 del; 24 mod Patch: https://git.openjdk.java.net/jdk/pull/2300.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2300/head:pull/2300 PR: https://git.openjdk.java.net/jdk/pull/2300
Re: RFR: 8189198: Add "forRemoval = true" to Applet API deprecations
On Wed, 17 Mar 2021 16:44:19 GMT, Andy Herrick wrote: > I cannot find any instances where the scope can be narrowed Are you about AquaInternalFrameUI.mouseRelased, BasicPopupMenuUI. stateChanged, and other non-trivial methods? - PR: https://git.openjdk.java.net/jdk/pull/2920
Re: RFR: 8260605: Various java.lang.invoke cleanups [v6]
On Wed, 17 Mar 2021 17:57:08 GMT, Claes Redestad wrote: >> - Remove unused code >> - Inline and simplify the bootstrap method invocation code (remove pointless >> reboxing checks etc) >> - Apply pattern matching to make some code more readable > > Claes Redestad has updated the pull request incrementally with one additional > commit since the last revision: > > Mandy review + additional cleanup src/java.base/share/classes/java/lang/invoke/InvokerBytecodeGenerator.java line 88: > 86: /** Name of new class */ > 87: private final String className; > 88: private final String prefixedClassName; Adding a field for the class name is a good change. I was tempted to rename `className` at one point to `name` which can be a class name in internal form or a simple name. It now makes more sense to do the renaming `s/className/name` and `s/prefixedClassName/className`. What do you think? - PR: https://git.openjdk.java.net/jdk/pull/2300
Re: RFR: 8263108: Class initialization deadlock in java.lang.constant [v5]
On Tue, 16 Mar 2021 14:47:29 GMT, Jaikiran Pai wrote: >> Can I please get a review for this proposed patch for the issue reported in >> https://bugs.openjdk.java.net/browse/JDK-8263108? >> >> As noted in that issue, the `java.lang.constant.DynamicConstantDesc` and >> `java.lang.constant.ConstantDescs` can end up in a classloading deadlock due >> to the nature of the code involved in their static blocks. A thread dump of >> one such deadlock (reproduced using the code attached to that issue) is as >> follows: >> >> "Thread A" #14 prio=5 os_prio=31 cpu=101.45ms elapsed=8.30s >> tid=0x7ff88e01ca00 nid=0x6003 in Object.wait() [0x7a4c6000] >>java.lang.Thread.State: RUNNABLE >> at >> java.lang.constant.DynamicConstantDesc.(java.base@16-ea/DynamicConstantDesc.java:67) >> - waiting on the Class initialization monitor for >> java.lang.constant.ConstantDescs >> at Deadlock.threadA(Deadlock.java:14) >> at Deadlock$$Lambda$1/0x000800c00a08.run(Unknown Source) >> at java.lang.Thread.run(java.base@16-ea/Thread.java:831) >> >> "Thread B" #15 prio=5 os_prio=31 cpu=103.15ms elapsed=8.30s >> tid=0x7ff88b936a00 nid=0x9b03 in Object.wait() [0x7a5c9000] >>java.lang.Thread.State: RUNNABLE >> at >> java.lang.constant.ClassDesc.ofDescriptor(java.base@16-ea/ClassDesc.java:145) >> - waiting on the Class initialization monitor for >> java.lang.constant.DynamicConstantDesc >> at >> java.lang.constant.ConstantDescs.(java.base@16-ea/ConstantDescs.java:239) >> at Deadlock.threadB(Deadlock.java:24) >> at Deadlock$$Lambda$2/0x000800c00c28.run(Unknown Source) >> at java.lang.Thread.run(java.base@16-ea/Thread.java:831) >> >> The commit in this PR resolves that deadlock by moving the `canonicalMap` >> initialization in `DynamicConstantDesc`, from the static block, to a lazily >> initialized map, into the `tryCanonicalize` (private) method of the same >> class. That's the only method which uses this map. >> >> The implementation in `tryCanonicalize` carefully ensures that the deadlock >> isn't shifted from the static block to this method, by making sure that the >> `synchronization` on `DynamicConstantDesc` in this method happens only when >> it's time to write the state to the shared `canonicalMap`. This has an >> implication that the method local variable `canonDescs` can get initialized >> by multiple threads unnecessarily but from what I can see, that's the only >> way we can avoid this deadlock. This penalty of multiple threads >> initializing and then throwing away that map isn't too huge because that >> will happen only once when the `canonicalMap` is being initialized for the >> first time. >> >> An alternate approach that I thought of was to completely get rid of this >> shared cache `canonicalMap` and instead just use method level map (that gets >> initialized each time) in the `tryCanonicalize` method (thus requiring no >> locks/synchronization). I ran a JMH benchmark with the current change >> proposed in this PR and with the alternate approach of using the method >> level map. The performance number from that run showed that the approach of >> using the method level map has relatively big impact on performance (even >> when there's a cache hit in that method). So I decided to not pursue that >> approach. I'll include the benchmark code and the numbers in a comment in >> this PR, for reference. >> >> The commit in this PR also includes a jtreg testcase which (consistently) >> reproduces the deadlock without the fix and passes when this fix is applied. >> Extra manual testing has been done to verify that the classes of interest >> (noted in that testcase) are indeed getting loaded in those concurrent >> threads and not in the main thread. For future maintainers, there's a >> implementation note added on that testcase explaining why it cannot be moved >> into testng test. > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > Add a comment to instruct future maintainers of the code to avoid calling > DynamicConstantDesc.ofCanonical() from static initialization of > java.lang.constant.ConstantDescs Marked as reviewed by chegar (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/2893
Re: RFR: 8260605: Various java.lang.invoke cleanups [v6]
On Wed, 17 Mar 2021 18:02:07 GMT, Claes Redestad wrote:
>> src/java.base/share/classes/java/lang/invoke/MethodType.java line 418:
>>
>>> 416: public MethodType changeParameterType(int num, Class nptype) {
>>> 417: if (parameterType(num) == nptype) return this;
>>> 418: checkPtype(nptype);
>>
>> `nptype` is never void but what about the check if `nptype` is not null?
>
> Other methods that delegate to `makeImpl` aren't doing up-front validation,
> so this change was made to get things more in line. It might be good to spell
> out that `makeImpl` does these checks for all its callers, though. (The
> `makeImpl` fast-path that execute before the validation can never return an
> invalid MethodType)
Generally we have a public API implementation to check the arguments upfront
for readability. In particular for this case, the validation cost is
negligible and removing the validation makes the code unclear where the
validation is done. I prefer to keep the validation there. It should check
that `nptype` is non-null and not `void.class`.
-
PR: https://git.openjdk.java.net/jdk/pull/2300
Re: RFR: 8260605: Various java.lang.invoke cleanups [v6]
On Mon, 15 Mar 2021 18:27:04 GMT, Mandy Chung wrote:
>> Claes Redestad has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Mandy review + additional cleanup
>
> src/java.base/share/classes/java/lang/invoke/MethodType.java line 418:
>
>> 416: public MethodType changeParameterType(int num, Class nptype) {
>> 417: if (parameterType(num) == nptype) return this;
>> 418: checkPtype(nptype);
>
> `nptype` is never void but what about the check if `nptype` is not null?
Other methods that delegate to `makeImpl` aren't doing up-front validation, so
this change was made to get things more in line. It might be good to spell out
that `makeImpl` does these checks for all its callers, though. (The `makeImpl`
fast-path that execute before the validation can never return an invalid
MethodType)
-
PR: https://git.openjdk.java.net/jdk/pull/2300
Re: RFR: 8260605: Various java.lang.invoke cleanups [v5]
On Mon, 15 Mar 2021 18:10:56 GMT, Mandy Chung wrote:
>> Claes Redestad has updated the pull request with a new target base due to a
>> merge or a rebase. The incremental webrev excludes the unrelated changes
>> brought in by the merge/rebase. The pull request contains seven additional
>> commits since the last revision:
>>
>> - Revert pattern matching changes covered by #2913
>> - Merge branch 'master' into invoke_cleanup
>> - Missing .values
>> - More cleanup, reduce allocations in InvokerBytecodeGenerator
>> - Missing outstanding changes
>> - Avoid unnecessary reboxing checks, inline and simplify class/mt invokes
>> - j.l.invoke cleanups
>
> src/java.base/share/classes/java/lang/invoke/BootstrapMethodInvoker.java line
> 151:
>
>> 149: maybeReBoxElements(argv);
>> 150: if (argv.length > 6) {
>> 151: result =
>> invokeWithManyArguments(bootstrapMethod, caller, name, type, argv);
>
> it'd be cleaner to move this to the default case in line 162 and 174 instead
> of having this special if-block.
Good catch!
> src/java.base/share/classes/java/lang/invoke/InvokerBytecodeGenerator.java
> line 342:
>
>> 340: setClassWriter(cw);
>> 341: cw.visit(Opcodes.V1_8, NOT_ACC_PUBLIC + Opcodes.ACC_FINAL +
>> Opcodes.ACC_SUPER,
>> 342: CLASS_PREFIX.concat(className), null,
>> INVOKER_SUPER_NAME, null);
>
> I prefer to use the existing common pattern using `+` as I believe this gain
> is in the noise.
The goal here was to remove/reduce allocations and excess calls - and using
String.concat might have been excessive.
The bulk of the overhead is the potentially many `CLASS_PREFIX + className`
calls, often via `className()`. Calculating this once and storing it in an
instance field `prefixedClassName` is more profitable and help better
disambiguates between `className` and `className()` - which sound like they
should be the same but aren't.
-
PR: https://git.openjdk.java.net/jdk/pull/2300
Re: RFR: 8260605: Various java.lang.invoke cleanups [v6]
> - Remove unused code > - Inline and simplify the bootstrap method invocation code (remove pointless > reboxing checks etc) > - Apply pattern matching to make some code more readable Claes Redestad has updated the pull request incrementally with one additional commit since the last revision: Mandy review + additional cleanup - Changes: - all: https://git.openjdk.java.net/jdk/pull/2300/files - new: https://git.openjdk.java.net/jdk/pull/2300/files/b20073e3..62ec7a40 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=2300=05 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2300=04-05 Stats: 56 lines in 2 files changed: 11 ins; 17 del; 28 mod Patch: https://git.openjdk.java.net/jdk/pull/2300.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2300/head:pull/2300 PR: https://git.openjdk.java.net/jdk/pull/2300
Re: RFR: 8263729: [test] Extend time to wait before destroying child in ProcssBuilder Basic test
On Wed, 17 Mar 2021 17:30:25 GMT, Thomas Stuefe wrote: >> Arbitrary time out has been a reliable source of intermittent failures. >> >> Since we have spent a lot of time analyzing this failure, I think it's >> worthwhile to fix it properly, which doesn't seem that complicated. That's >> better than the same bug happening again a year later and a different set of >> people would spend hours to analyze it again. > > I don't think this is CPU starvation but memory exhaustion. _beginthreadex > fails with EACCES if it has no resources to start the thread, which in this > case probably means memory (the other possibility would be > out-of-HANDLE-space but seeing that the child just started I don't see how > this could be). > > Should we harden tests against resource starvation like this, or rather > require the test machine to be beefy enough for tests? Also, I don't > understand, if the child has not enough resources to bring the VM fully up > how waiting on either stream would help. @tstuefe It's unlikely that _beginthreadex failed due to lack of memory. We are running on a machine with more than 50GB ram with only concurrency of 6. Extracts from the logs: $ jtreg -vmoption:-Xmx512m -concurrency:6 -vmoption:-XX:MaxRAMPercentage=4 open/test/jdk:jdk_lang start = Wed Mar 10 22:54:53 GMT 2021 end = Wed Mar 10 22:56:36 GMT 2021 elapsed= 102932 0:01:42.932 [2021-03-10 22:57:07] [C:\cygwin\bin\free.exe] timeout=2 total used free shared buff/cache available Mem: 50068964 19974888 30094076 0 0 30094076 Swap: 8388608 105048 8283560 [2021-03-10 22:57:07] exit code: 0 time: 33 ms My theory is that `TerminateProcess` has made it impossible for the child process to spawn new threads, but somehow existing threads are still able to run a little bit and produce the log message. Of course this is just a theory, and I cannot find any supporting docs from MS. However, if we implement the work around and make sure we don't kill the child until it has finished bootstrapping, and the bug doesn't happen anymore, then we know something more. - PR: https://git.openjdk.java.net/jdk/pull/3049
Re: RFR: 8263729: [test] Extend time to wait before destroying child in ProcssBuilder Basic test
On Wed, 17 Mar 2021 17:30:25 GMT, Thomas Stuefe wrote: >> Arbitrary time out has been a reliable source of intermittent failures. >> >> Since we have spent a lot of time analyzing this failure, I think it's >> worthwhile to fix it properly, which doesn't seem that complicated. That's >> better than the same bug happening again a year later and a different set of >> people would spend hours to analyze it again. > > I don't think this is CPU starvation but memory exhaustion. _beginthreadex > fails with EACCES if it has no resources to start the thread, which in this > case probably means memory (the other possibility would be > out-of-HANDLE-space but seeing that the child just started I don't see how > this could be). > > Should we harden tests against resource starvation like this, or rather > require the test machine to be beefy enough for tests? Also, I don't > understand, if the child has not enough resources to bring the VM fully up > how waiting on either stream would help. I'm not sure what part of the system or tests should be more robust. If the VM can't tolerate the failure of a thread to start, then it should be a fatal VM error, not just an unexpected line on stdout (or it should be able to proceed without the extra thread). It is a reasonable position to take that when a process is destroyed, there can be unpredictable effects. Though in various iterations, attempts have been made to shutdown in an orderly fashion, coupled to calls to System.exit() or SIGTERM. (It would be nice if Windows had a way to do a cleanly request process termination - a SIGTERM equivalent does not exist.) Avoidance may be easiest but may just hide another problem. - PR: https://git.openjdk.java.net/jdk/pull/3049
Re: RFR: 8263729: [test] Extend time to wait before destroying child in ProcssBuilder Basic test
On Wed, 17 Mar 2021 17:14:22 GMT, Ioi Lam wrote: >> That complicates the test and the child quite a bit for minimal gain. > > Arbitrary time out has been a reliable source of intermittent failures. > > Since we have spent a lot of time analyzing this failure, I think it's > worthwhile to fix it properly, which doesn't seem that complicated. That's > better than the same bug happening again a year later and a different set of > people would spend hours to analyze it again. I don't think this is CPU starvation but memory exhaustion. _beginthreadex fails with EACCES if it has no resources to start the thread, which in this case probably means memory (the other possibility would be out-of-HANDLE-space but seeing that the child just started I don't see how this could be). Should we harden tests against resource starvation like this, or rather require the test machine to be beefy enough for tests? Also, I don't understand, if the child has not enough resources to bring the VM fully up how waiting on either stream would help. - PR: https://git.openjdk.java.net/jdk/pull/3049
Re: RFR: 8263729: [test] Extend time to wait before destroying child in ProcssBuilder Basic test
On Wed, 17 Mar 2021 16:40:47 GMT, Roger Riggs wrote: >> The failures happened in tiers 6 and 8. The system may be overloaded so even >> 100ms may not be enough for the child process to start sleeping. From the >> error log, the child process tried to spawn a thread (probably one of those >> usually started during VM bootstrap) at around 118ms >> >> [0.118s][warning][os,thread] Failed to start thread - _beginthreadex failed >> (EACCES) for attributes: stacksize: default, flags: >> >> The test runs 4 times. Each time it checks only STDOUT or STDERR, but not >> both. So I think we can use the other stream to signal to the main process >> that the child process is ready. That would be more reliable than an >> arbitrary wait time. > > That complicates the test and the child quite a bit for minimal gain. Arbitrary time out has been a reliable source of intermittent failures. Since we have spent a lot of time analyzing this failure, I think it's worthwhile to fix it properly, which doesn't seem that complicated. That's better than the same bug happening again a year later and a different set of people would spend hours to analyze it again. - PR: https://git.openjdk.java.net/jdk/pull/3049
Integrated: 8263726: divideToIntegralValue typo on BigDecimal documentation
On Wed, 17 Mar 2021 16:43:42 GMT, Joe Darcy wrote: > Fix typos in references to the method's name in the javadoc. This pull request has now been integrated. Changeset: 24afa36d Author:Joe Darcy URL: https://git.openjdk.java.net/jdk/commit/24afa36d Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod 8263726: divideToIntegralValue typo on BigDecimal documentation Reviewed-by: bpb - PR: https://git.openjdk.java.net/jdk/pull/3051
Re: RFR: 8251942: PrintStream specification is not clear which flush method is automatically invoked
On Thu, 11 Mar 2021 20:54:12 GMT, Brian Burkhalter wrote:
>>> Yes, I noticed that as well. I didn?t think it was worth complicating
>>> things for the purpose of this issue to address it.
>>
>> I guess what I was trying to ask is whether we should actually specify that
>> `print` and `append` call `flush` - as this seems to be a side effect of
>> some optimization.
>> Maybe we should say that the implementation ensures that flush is called
>> when writing a byte array or when a newline character or byte ({@code
>> '\n'}) is written - but might call it in additional unspecified
>> circumstances?
>
> I think you are correct. In the second commit I scaled back the change to the
> class level specification. I left out any mention of "unspecified
> circumstances."
Is there any further comment on this request? Thanks.
-
PR: https://git.openjdk.java.net/jdk/pull/2926
Re: RFR: 8263726: divideToIntegralValue typo on BigDecimal documentation
On Wed, 17 Mar 2021 16:43:42 GMT, Joe Darcy wrote: > Fix typos in references to the method's name in the javadoc. Looks fine. - Marked as reviewed by bpb (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/3051
RFR: 8263726: divideToIntegralValue typo on BigDecimal documentation
Fix typos in references to the method's name in the javadoc. - Commit messages: - 8263726: divideToIntegralValue typo on BigDecimal documentation Changes: https://git.openjdk.java.net/jdk/pull/3051/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=3051=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8263726 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/3051.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3051/head:pull/3051 PR: https://git.openjdk.java.net/jdk/pull/3051
Re: RFR: 8189198: Add "forRemoval = true" to Applet API deprecations
On Sun, 14 Mar 2021 12:06:08 GMT, Alan Bateman wrote: > > > Have you looked at narrowing the use of the SuppressWarnings to local > variable declarations to avoid adding it to some of these methods? in all cases either: - the class or method itself is being deprecated - the method takes a deprecated arg . - there is no local variable involved, such as testing if something is an instanceOf a class being deprecated, or calling a deprecated method. I cannot find any instances where the scope can be narrowed - PR: https://git.openjdk.java.net/jdk/pull/2920
Re: RFR: 8263729: [test] Extend time to wait before destroying child in ProcssBuilder Basic test
On Wed, 17 Mar 2021 16:36:19 GMT, Ioi Lam wrote: >> That loop is checking that the Thread (in the parent) reading from the child >> is in the correct state (blocked). On Windows, it is a BufferedInputStream. >> >> But it does not indicate anything about the state of the child process. >> From the scant information from previous failures, it appears that the the >> creation of some thread (not a java thread) in the child has failed. >> The additional time is to avoid destroying the child while it is still >> initializing. > > The failures happened in tiers 6 and 8. The system may be overloaded so even > 100ms may not be enough for the child process to start sleeping. From the > error log, the child process tried to spawn a thread (probably one of those > usually started during VM bootstrap) at around 118ms > > [0.118s][warning][os,thread] Failed to start thread - _beginthreadex failed > (EACCES) for attributes: stacksize: default, flags: > > The test runs 4 times. Each time it checks only STDOUT or STDERR, but not > both. So I think we can use the other stream to signal to the main process > that the child process is ready. That would be more reliable than an > arbitrary wait time. That complicates the test and the child quite a bit for minimal gain. - PR: https://git.openjdk.java.net/jdk/pull/3049
Re: RFR: 8254979: Class.getSimpleName() returns non-empty for lambda and method
On Tue, 16 Mar 2021 22:24:55 GMT, Joe Darcy wrote: > java.lang.Class has a number of methods to return some kind of textual token > associated with the class, including a type name, canonical name, simple > name, and separately the results of toString(). > > Bug 8254979 notes that getSimpleName mentions returning the name in source > code, but operationally returns a non-null result for synthetic classes, ones > without benefit of source code representation. Since it is useful to return a > non-null result in such cases, I've updated the spec to mention this case. > The names of synthetic classes are not covered by the JLS; using a "$" in > such names is customary, but not strictly required. The synthetic classes > used to implement lambdas and method references as cited in the bug report > include "$"'s. > > Looking over the other "getFooName" methods, I don't think they need to be > updated to handle this case. Marked as reviewed by mchung (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/3038
Re: RFR: 8263729: [test] Extend time to wait before destroying child in ProcssBuilder Basic test
On Wed, 17 Mar 2021 15:18:41 GMT, Roger Riggs wrote:
>> test/jdk/java/lang/ProcessBuilder/Basic.java line 2183:
>>
>>> 2181: latch.await();
>>> 2182: Thread.sleep(100L); // Wait for child
>>> initialization to settle
>>> 2183:
>>
>> Hi Roger,
>> Shouldn't the code that follows this sleep already be enough to wait for
>> child thread to get a lock on the stream?
>>
>> Thread.sleep(100L); // Wait for child initialization to
>> settle
>>
>> if (s instanceof BufferedInputStream) {
>> // Wait until after the s.read occurs in "thread" by
>> // checking when the input stream monitor is acquired
>> // (BufferedInputStream.read is synchronized)
>> while (!isLocked(s, 10)) {
>> Thread.sleep(100);
>> }
>> }
>>
>>
>> From 1st glance I would conclude that the 1st sleep is unnecessary. Is there
>> a platform where Input/Error stream is not a BufferedInputStream? Or is it
>> calling Process.destory() immediately after the child thread enters
>> BufferedInputStream.read synchronized method perhaps too early to trigger
>> appropriate exception in the child thread? In that case the additional sleep
>> should be added after the while(!isLocked... loop.
>
> That loop is checking that the Thread (in the parent) reading from the child
> is in the correct state (blocked). On Windows, it is a BufferedInputStream.
>
> But it does not indicate anything about the state of the child process.
> From the scant information from previous failures, it appears that the the
> creation of some thread (not a java thread) in the child has failed.
> The additional time is to avoid destroying the child while it is still
> initializing.
The failures happened in tiers 6 and 8. The system may be overloaded so even
100ms may not be enough for the child process to start sleeping. From the error
log, the child process tried to spawn a thread (probably one of those usually
started during VM bootstrap) at around 118ms
[0.118s][warning][os,thread] Failed to start thread - _beginthreadex failed
(EACCES) for attributes: stacksize: default, flags:
The test runs 4 times. Each time it checks only STDOUT or STDERR, but not both.
So I think we can use the other stream to signal to the main process that the
child process is ready. That would be more reliable than an arbitrary wait time.
-
PR: https://git.openjdk.java.net/jdk/pull/3049
Re: RFR: 8254979: Class.getSimpleName() returns non-empty for lambda and method
On Tue, 16 Mar 2021 22:24:55 GMT, Joe Darcy wrote: > java.lang.Class has a number of methods to return some kind of textual token > associated with the class, including a type name, canonical name, simple > name, and separately the results of toString(). > > Bug 8254979 notes that getSimpleName mentions returning the name in source > code, but operationally returns a non-null result for synthetic classes, ones > without benefit of source code representation. Since it is useful to return a > non-null result in such cases, I've updated the spec to mention this case. > The names of synthetic classes are not covered by the JLS; using a "$" in > such names is customary, but not strictly required. The synthetic classes > used to implement lambdas and method references as cited in the bug report > include "$"'s. > > Looking over the other "getFooName" methods, I don't think they need to be > updated to handle this case. Marked as reviewed by rriggs (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/3038
Re: RFR: 8261785: Calling "main" method in anonymous nested class crashes the JVM [v3]
On Wed, 17 Mar 2021 08:55:54 GMT, Alan Bateman wrote: > > > Using an anonymous class for the main class looks strange and hard to > > > believe anyone is relying on this. I wonder if we should do more checking > > > LauncherHelper.validateMainClass to reject cases like this. > > > > > > I raised that same question, and people tends to agree launcher could > > reject anonymous/local classes. But pointed out that should require a CSR > > review. Therefore I chose to fix crash first, and we can file another > > ticket to address main class requirements. > > The requirement for a CSR and release note should not be a concern here. I > don't object to the fix but I think it would be very useful to document the > main class and whether local or anonymous classes can be used, its > accessibility, and the accessibility of the main method. We had to do > something similar recently with the premain method (java.lang.instrument). Absolutely we need to clarify that, however, the discussion of what should or should not be allowed may take some time. For example, if we limit such usage in launcher, should it be possible for custom launcher to start such a main method? Thus the recommendation to have a separate ticket for that. The current java document mostly only say to load the specify the class name, however there is a sentence `By default, the first argument that isn't an option of the java command is the fully qualified name of the class to be called. If -jar is specified, then its argument is the name of the JAR file containing class and resource files for the application. The startup class must be indicated by the Main-Class manifest header in its manifest file.`. Not that it says `fully qualified name of the class`. Filed [JDK-8263735](https://bugs.openjdk.java.net/browse/JDK-8263735) for the follow up, in the mean time, we should get this crash resolved. - PR: https://git.openjdk.java.net/jdk/pull/2999
Withdrawn: 8189198: Add "forRemoval = true" to Applet API deprecations
On Wed, 10 Mar 2021 18:33:37 GMT, Andy Herrick wrote: > implementation of > JDK-8256145: JEP 398: Deprecate the Applet API for Removal This pull request has been closed without being integrated. - PR: https://git.openjdk.java.net/jdk/pull/2920
Re: RFR: 8189198: Add "forRemoval = true" to Applet API deprecations
On Sun, 14 Mar 2021 12:06:08 GMT, Alan Bateman wrote: > the - PR: https://git.openjdk.java.net/jdk/pull/2920
Re: RFR: 8263729: [test] Extend time to wait before destroying child in ProcssBuilder Basic test
On Wed, 17 Mar 2021 14:52:36 GMT, Peter Levart wrote:
>> Intermittent failures on Windows in a test of destroying the child warrant
>> extending the time the parent waits after starting the child before
>> destroying the child.
>
> test/jdk/java/lang/ProcessBuilder/Basic.java line 2183:
>
>> 2181: latch.await();
>> 2182: Thread.sleep(100L); // Wait for child
>> initialization to settle
>> 2183:
>
> Hi Roger,
> Shouldn't the code that follows this sleep already be enough to wait for
> child thread to get a lock on the stream?
>
> Thread.sleep(100L); // Wait for child initialization to
> settle
>
> if (s instanceof BufferedInputStream) {
> // Wait until after the s.read occurs in "thread" by
> // checking when the input stream monitor is acquired
> // (BufferedInputStream.read is synchronized)
> while (!isLocked(s, 10)) {
> Thread.sleep(100);
> }
> }
>
>
> From 1st glance I would conclude that the 1st sleep is unnecessary. Is there
> a platform where Input/Error stream is not a BufferedInputStream? Or is it
> calling Process.destory() immediately after the child thread enters
> BufferedInputStream.read synchronized method perhaps too early to trigger
> appropriate exception in the child thread? In that case the additional sleep
> should be added after the while(!isLocked... loop.
That loop is checking that the Thread (in the parent) reading from the child is
in the correct state (blocked). On Windows, it is a BufferedInputStream.
But it does not indicate anything about the state of the child process.
>From the scant information from previous failures, it appears that the the
>creation of some thread (not a java thread) in the child has failed.
The additional time is to avoid destroying the child while it is still
initializing.
-
PR: https://git.openjdk.java.net/jdk/pull/3049
Re: RFR: 8263108: Class initialization deadlock in java.lang.constant [v5]
On Wed, 17 Mar 2021 14:34:41 GMT, Peter Levart wrote: >> Jaikiran Pai has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Add a comment to instruct future maintainers of the code to avoid calling >> DynamicConstantDesc.ofCanonical() from static initialization of >> java.lang.constant.ConstantDescs > > This looks good, Jaikiran. Thank you Peter, Chris and Vyom for the reviews. I'll go ahead and integrate this in a few hours. - PR: https://git.openjdk.java.net/jdk/pull/2893
Re: RFR: 8263729: [test] Extend time to wait before destroying child in ProcssBuilder Basic test
On Wed, 17 Mar 2021 14:16:36 GMT, Roger Riggs wrote:
> Intermittent failures on Windows in a test of destroying the child warrant
> extending the time the parent waits after starting the child before
> destroying the child.
test/jdk/java/lang/ProcessBuilder/Basic.java line 2183:
> 2181: latch.await();
> 2182: Thread.sleep(100L); // Wait for child
> initialization to settle
> 2183:
Hi Roger,
Shouldn't the code that follows this sleep already be enough to wait for child
thread to get a lock on the stream?
Thread.sleep(100L); // Wait for child initialization to
settle
if (s instanceof BufferedInputStream) {
// Wait until after the s.read occurs in "thread" by
// checking when the input stream monitor is acquired
// (BufferedInputStream.read is synchronized)
while (!isLocked(s, 10)) {
Thread.sleep(100);
}
}
>From 1st glance I would conclude that the 1st sleep is unnecessary. Is there a
>platform where Input/Error stream is not a BufferedInputStream?
-
PR: https://git.openjdk.java.net/jdk/pull/3049
Re: RFR: 8263108: Class initialization deadlock in java.lang.constant [v5]
On Tue, 16 Mar 2021 14:47:29 GMT, Jaikiran Pai wrote: >> Can I please get a review for this proposed patch for the issue reported in >> https://bugs.openjdk.java.net/browse/JDK-8263108? >> >> As noted in that issue, the `java.lang.constant.DynamicConstantDesc` and >> `java.lang.constant.ConstantDescs` can end up in a classloading deadlock due >> to the nature of the code involved in their static blocks. A thread dump of >> one such deadlock (reproduced using the code attached to that issue) is as >> follows: >> >> "Thread A" #14 prio=5 os_prio=31 cpu=101.45ms elapsed=8.30s >> tid=0x7ff88e01ca00 nid=0x6003 in Object.wait() [0x7a4c6000] >>java.lang.Thread.State: RUNNABLE >> at >> java.lang.constant.DynamicConstantDesc.(java.base@16-ea/DynamicConstantDesc.java:67) >> - waiting on the Class initialization monitor for >> java.lang.constant.ConstantDescs >> at Deadlock.threadA(Deadlock.java:14) >> at Deadlock$$Lambda$1/0x000800c00a08.run(Unknown Source) >> at java.lang.Thread.run(java.base@16-ea/Thread.java:831) >> >> "Thread B" #15 prio=5 os_prio=31 cpu=103.15ms elapsed=8.30s >> tid=0x7ff88b936a00 nid=0x9b03 in Object.wait() [0x7a5c9000] >>java.lang.Thread.State: RUNNABLE >> at >> java.lang.constant.ClassDesc.ofDescriptor(java.base@16-ea/ClassDesc.java:145) >> - waiting on the Class initialization monitor for >> java.lang.constant.DynamicConstantDesc >> at >> java.lang.constant.ConstantDescs.(java.base@16-ea/ConstantDescs.java:239) >> at Deadlock.threadB(Deadlock.java:24) >> at Deadlock$$Lambda$2/0x000800c00c28.run(Unknown Source) >> at java.lang.Thread.run(java.base@16-ea/Thread.java:831) >> >> The commit in this PR resolves that deadlock by moving the `canonicalMap` >> initialization in `DynamicConstantDesc`, from the static block, to a lazily >> initialized map, into the `tryCanonicalize` (private) method of the same >> class. That's the only method which uses this map. >> >> The implementation in `tryCanonicalize` carefully ensures that the deadlock >> isn't shifted from the static block to this method, by making sure that the >> `synchronization` on `DynamicConstantDesc` in this method happens only when >> it's time to write the state to the shared `canonicalMap`. This has an >> implication that the method local variable `canonDescs` can get initialized >> by multiple threads unnecessarily but from what I can see, that's the only >> way we can avoid this deadlock. This penalty of multiple threads >> initializing and then throwing away that map isn't too huge because that >> will happen only once when the `canonicalMap` is being initialized for the >> first time. >> >> An alternate approach that I thought of was to completely get rid of this >> shared cache `canonicalMap` and instead just use method level map (that gets >> initialized each time) in the `tryCanonicalize` method (thus requiring no >> locks/synchronization). I ran a JMH benchmark with the current change >> proposed in this PR and with the alternate approach of using the method >> level map. The performance number from that run showed that the approach of >> using the method level map has relatively big impact on performance (even >> when there's a cache hit in that method). So I decided to not pursue that >> approach. I'll include the benchmark code and the numbers in a comment in >> this PR, for reference. >> >> The commit in this PR also includes a jtreg testcase which (consistently) >> reproduces the deadlock without the fix and passes when this fix is applied. >> Extra manual testing has been done to verify that the classes of interest >> (noted in that testcase) are indeed getting loaded in those concurrent >> threads and not in the main thread. For future maintainers, there's a >> implementation note added on that testcase explaining why it cannot be moved >> into testng test. > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > Add a comment to instruct future maintainers of the code to avoid calling > DynamicConstantDesc.ofCanonical() from static initialization of > java.lang.constant.ConstantDescs This looks good, Jaikiran. - Marked as reviewed by plevart (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/2893
RFR: 8263729: [test] Extend time to wait before destroying child in ProcssBuilder Basic test
Intermittent failures on Windows in a test of destroying the child warrant extending the time the parent waits after starting the child before destroying the child. - Commit messages: - 8263729: [test] Extend time to wait before destroying child in ProcessBuilder Basic test Changes: https://git.openjdk.java.net/jdk/pull/3049/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=3049=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8263729 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/3049.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3049/head:pull/3049 PR: https://git.openjdk.java.net/jdk/pull/3049
Integrated: 8148937: (str) Adapt StringJoiner for Compact Strings
On Thu, 18 Feb 2021 14:30:15 GMT, Сергей Цыпанов
wrote:
> Hello,
>
> as of now `java.util.StringJoiner` still uses `char[]` as a storage for
> joined Strings.
>
> This applies for the cases when all joined Strings as well as delimiter,
> prefix and suffix contain only ASCII symbols.
>
> As a result when `StringJoiner.toString()` is called `byte[]` stored in
> Strings is inflated in order to fill in `char[]` and after that `char[]` is
> compressed when constructor of String is called:
> String delimiter = this.delimiter;
> char[] chars = new char[this.len + addLen];
> int k = getChars(this.prefix, chars, 0);
> if (size > 0) {
> k += getChars(elts[0], chars, k);// inflate byte[]
>
> for(int i = 1; i < size; ++i) {
> k += getChars(delimiter, chars, k);
> k += getChars(elts[i], chars, k);
> }
> }
>
> k += getChars(this.suffix, chars, k);
> return new String(chars);// compress char[] -> byte[]
> This can be improved by utilizing new method `String.getBytes(byte[], int,
> int, byte, int)` [introduced](https://github.com/openjdk/jdk/pull/402) in
> [JDK-8224986](https://bugs.openjdk.java.net/browse/JDK-8254082)
> covering both cases when resulting String is Latin1 or UTF-16
>
> I've prepared a patch along with benchmark proving that this change is
> correct and brings improvement.
>
> @BenchmarkMode(Mode.AverageTime)
> @OutputTimeUnit(TimeUnit.NANOSECONDS)
> @Fork(jvmArgsAppend = {"-Xms2g", "-Xmx2g"})
> public class StringJoinerBenchmark {
>
> @Benchmark
> public String stringJoiner(Data data) {
> String[] stringArray = data.stringArray;
> return Joiner.joinWithStringJoiner(stringArray);
> }
>
> @State(Scope.Thread)
> public static class Data {
>
> @Param({"latin", "cyrillic", "mixed"})
> private String mode;
>
> @Param({"8", "32", "64"})
> private int length;
>
> @Param({"5", "10", "100"})
> private int count;
>
> private String[] stringArray;
>
> @Setup
> public void setup() {
> RandomStringGenerator generator = new RandomStringGenerator();
>
> stringArray = new String[count];
>
> for (int i = 0; i < count; i++) {
> String alphabet = getAlphabet(i, mode);
> stringArray[i] = generator.randomString(alphabet, length);
> }
> }
>
> private static String getAlphabet(int index, String mode) {
> var latin = "abcdefghijklmnopqrstuvwxyz"; //English
> var cyrillic = "абвгдеёжзиклмнопрстуфхцчшщьыъэюя"; // Russian
>
> String alphabet;
> switch (mode) {
> case "mixed" -> alphabet = index % 2 == 0 ? cyrillic : latin;
> case "latin" -> alphabet = latin;
> case "cyrillic" -> alphabet = cyrillic;
> default -> throw new RuntimeException("Illegal mode " + mode);
> }
> return alphabet;
> }
> }
> }
>
> public class Joiner {
>
> public static String joinWithStringJoiner(String[] stringArray) {
> StringJoiner joiner = new StringJoiner(",", "[", "]");
> for (String str : stringArray) {
> joiner.add(str);
> }
> return joiner.toString();
> }
> }
>
>
> (count) (length)(mode)
> Java 14 patched Units
> stringJoiner 5 8 latin 78.836
> ± 0.20867.546 ± 0.500 ns/op
> stringJoiner 532 latin 92.877
> ± 0.42266.760 ± 0.498 ns/op
> stringJoiner 564 latin 115.423
> ± 0.88373.224 ± 0.289 ns/op
> stringJoiner 10 8 latin 152.587
> ± 0.429 161.427 ± 0.635 ns/op
> stringJoiner 1032 latin 189.998
> ± 0.478 164.099 ± 0.963 ns/op
> stringJoiner 1064 latin 238.679
> ± 1.419 176.825 ± 0.533 ns/op
> stringJoiner100 8 latin 1215.612
> ± 17.413 1541.802 ± 126.166 ns/op
> stringJoiner10032 latin 1699.998
> ± 28.407 1563.341 ± 4.439 ns/op
> stringJoiner10064 latin 2289.388
> ± 45.319 2215.931 ± 137.583 ns/op
> stringJoiner 5 8 cyrillic 96.692
> ± 0.94780.946 ± 0.371 ns/op
> stringJoiner 532 cyrillic 107.806
> ± 0.42984.717 ± 0.541 ns/op
> stringJoiner 564 cyrillic 150.762
> ± 2.26796.214 ± 1.251 ns/op
> stringJoiner 10 8 cyrillic 190.570
> ± 0.381 182.754 ± 0.678 ns/op
> stringJoiner 1032 cyrillic
Re: RFR: 8148937: (str) Adapt StringJoiner for Compact Strings [v3]
On Mon, 15 Mar 2021 21:47:28 GMT, Сергей Цыпанов
wrote:
>> Hello,
>>
>> as of now `java.util.StringJoiner` still uses `char[]` as a storage for
>> joined Strings.
>>
>> This applies for the cases when all joined Strings as well as delimiter,
>> prefix and suffix contain only ASCII symbols.
>>
>> As a result when `StringJoiner.toString()` is called `byte[]` stored in
>> Strings is inflated in order to fill in `char[]` and after that `char[]` is
>> compressed when constructor of String is called:
>> String delimiter = this.delimiter;
>> char[] chars = new char[this.len + addLen];
>> int k = getChars(this.prefix, chars, 0);
>> if (size > 0) {
>> k += getChars(elts[0], chars, k);// inflate byte[]
>>
>> for(int i = 1; i < size; ++i) {
>> k += getChars(delimiter, chars, k);
>> k += getChars(elts[i], chars, k);
>> }
>> }
>>
>> k += getChars(this.suffix, chars, k);
>> return new String(chars);// compress char[] -> byte[]
>> This can be improved by utilizing new method `String.getBytes(byte[], int,
>> int, byte, int)` [introduced](https://github.com/openjdk/jdk/pull/402) in
>> [JDK-8224986](https://bugs.openjdk.java.net/browse/JDK-8254082)
>> covering both cases when resulting String is Latin1 or UTF-16
>>
>> I've prepared a patch along with benchmark proving that this change is
>> correct and brings improvement.
>>
>> @BenchmarkMode(Mode.AverageTime)
>> @OutputTimeUnit(TimeUnit.NANOSECONDS)
>> @Fork(jvmArgsAppend = {"-Xms2g", "-Xmx2g"})
>> public class StringJoinerBenchmark {
>>
>> @Benchmark
>> public String stringJoiner(Data data) {
>> String[] stringArray = data.stringArray;
>> return Joiner.joinWithStringJoiner(stringArray);
>> }
>>
>> @State(Scope.Thread)
>> public static class Data {
>>
>> @Param({"latin", "cyrillic", "mixed"})
>> private String mode;
>>
>> @Param({"8", "32", "64"})
>> private int length;
>>
>> @Param({"5", "10", "100"})
>> private int count;
>>
>> private String[] stringArray;
>>
>> @Setup
>> public void setup() {
>> RandomStringGenerator generator = new RandomStringGenerator();
>>
>> stringArray = new String[count];
>>
>> for (int i = 0; i < count; i++) {
>> String alphabet = getAlphabet(i, mode);
>> stringArray[i] = generator.randomString(alphabet, length);
>> }
>> }
>>
>> private static String getAlphabet(int index, String mode) {
>> var latin = "abcdefghijklmnopqrstuvwxyz"; //English
>> var cyrillic = "абвгдеёжзиклмнопрстуфхцчшщьыъэюя"; // Russian
>>
>> String alphabet;
>> switch (mode) {
>> case "mixed" -> alphabet = index % 2 == 0 ? cyrillic : latin;
>> case "latin" -> alphabet = latin;
>> case "cyrillic" -> alphabet = cyrillic;
>> default -> throw new RuntimeException("Illegal mode " + mode);
>> }
>> return alphabet;
>> }
>> }
>> }
>>
>> public class Joiner {
>>
>> public static String joinWithStringJoiner(String[] stringArray) {
>> StringJoiner joiner = new StringJoiner(",", "[", "]");
>> for (String str : stringArray) {
>> joiner.add(str);
>> }
>> return joiner.toString();
>> }
>> }
>>
>>
>> (count) (length)(mode)
>> Java 14 patched Units
>> stringJoiner 5 8 latin 78.836
>> ± 0.20867.546 ± 0.500 ns/op
>> stringJoiner 532 latin 92.877
>> ± 0.42266.760 ± 0.498 ns/op
>> stringJoiner 564 latin 115.423
>> ± 0.88373.224 ± 0.289 ns/op
>> stringJoiner 10 8 latin 152.587
>> ± 0.429 161.427 ± 0.635 ns/op
>> stringJoiner 1032 latin 189.998
>> ± 0.478 164.099 ± 0.963 ns/op
>> stringJoiner 1064 latin 238.679
>> ± 1.419 176.825 ± 0.533 ns/op
>> stringJoiner100 8 latin 1215.612
>> ± 17.413 1541.802 ± 126.166 ns/op
>> stringJoiner10032 latin 1699.998
>> ± 28.407 1563.341 ± 4.439 ns/op
>> stringJoiner10064 latin 2289.388
>> ± 45.319 2215.931 ± 137.583 ns/op
>> stringJoiner 5 8 cyrillic 96.692
>> ± 0.94780.946 ± 0.371 ns/op
>> stringJoiner 532 cyrillic 107.806
>> ± 0.42984.717 ± 0.541 ns/op
>> stringJoiner 564 cyrillic 150.762
>> ± 2.26796.214 ± 1.251 ns/op
>> stringJoiner 10 8 cyrillic
Re: RFR: 8263658: Use the blessed modifier order in java.base
On Sat, 13 Mar 2021 22:45:30 GMT, Alex Blewitt wrote: > Sonar displays a warning message that modifiers should be declared in the > order listed in the JLS; specifically, that isntead of using `final static` > the `static final` should be preferred. > > This fixes the issues in the `java.base` package for ease of reviewing. > > https://sonarcloud.io/project/issues?id=shipilev_jdk=java=false=java%3AS1124 Marked as reviewed by rriggs (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/2993
RFR: 8263658: Use the blessed modifier order in java.base
Sonar displays a warning message that modifiers should be declared in the order listed in the JLS; specifically, that isntead of using `final static` the `static final` should be preferred. This fixes the issues in the `java.base` package for ease of reviewing. https://sonarcloud.io/project/issues?id=shipilev_jdk=java=false=java%3AS1124 - Commit messages: - 8263658: Use the blessed modifier order in java.base Changes: https://git.openjdk.java.net/jdk/pull/2993/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=2993=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8263658 Stats: 140 lines in 29 files changed: 0 ins; 0 del; 140 mod Patch: https://git.openjdk.java.net/jdk/pull/2993.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2993/head:pull/2993 PR: https://git.openjdk.java.net/jdk/pull/2993
Re: RFR: 8263658: Use the blessed modifier order in java.base
On Sat, 13 Mar 2021 22:45:30 GMT, Alex Blewitt wrote: > Sonar displays a warning message that modifiers should be declared in the > order listed in the JLS; specifically, that isntead of using `final static` > the `static final` should be preferred. > > This fixes the issues in the `java.base` package for ease of reviewing. > > https://sonarcloud.io/project/issues?id=shipilev_jdk=java=false=java%3AS1124 Please change the synopsis to "8263658: Use the blessed modifier order in java.base" to get this PR hooked properly to the new bug. Also configure the run the testing, see "Pre-submit test status" in "Checks". - PR: https://git.openjdk.java.net/jdk/pull/2993
Withdrawn: 8259925: [Vector API] Unreasonable IndexOutOfBoundsException message when length < vlen
On Mon, 18 Jan 2021 13:32:24 GMT, Jie Fu wrote:
> Hi all,
>
> For this reproducer:
>
> import jdk.incubator.vector.ByteVector;
> import jdk.incubator.vector.VectorSpecies;
>
> public class Test {
> static final VectorSpecies SPECIES_128 = ByteVector.SPECIES_128;
> static byte[] a = new byte[8];
> static byte[] b = new byte[8];
>
> public static void main(String[] args) {
> ByteVector av = ByteVector.fromArray(SPECIES_128, a, 0);
> av.intoArray(b, 0);
> System.out.println("b: " + b[0]);
> }
> }
>
> The following IndexOutOfBoundsException message (length -7) is unreasonable.
>
> Exception in thread "main" java.lang.IndexOutOfBoundsException: Index 0 out
> of bounds for length -7
>
> It might be better to fix it like this.
>
> Exception in thread "main" java.lang.IndexOutOfBoundsException: Index 0 out
> of bounds for length 0
>
> Thanks.
> Best regards,
> Jie
This pull request has been closed without being integrated.
-
PR: https://git.openjdk.java.net/jdk/pull/2127
Re: java.io.File#toPath() on Windows doesn't understand "NUL:" (null device)?
On 17/03/21 3:10 pm, Jaikiran Pai wrote:
Hello Alan,
On 17/03/21 2:45 pm, Alan Bateman wrote:
On 17/03/2021 08:21, Jaikiran Pai wrote:
:
I can confirm that using "NUL" or "nul" work fine in the above code,
I don't know the context for your question
A while back Apache Ant switched to using the
Files.newInputStream/Files.newOutputStream construct as a replacement
for the FileInputStream and FileOutputStream constructors[1]. That
commit seems to have introduced an regression in Ant as noted here[2].
It appears that users of Ant were using "nul" (and even "nul:") as
destination file to have Ant write the data to that destination.
Internally Ant constructs an instance of java.io.File from the user
provided path string ("nul" or "nul:" in this case) and constructs a
OutputStream out of it. Previously (before we made that commit in
Ant), it would be using the FileOutputStream constructor (and would
succeed) and now it uses the Files.newOuputStream(...) which expects a
Path instance and this where our usage of java.io.File.toPath() ran
into the issue I note in this mail, when I started investigating this.
I didn't mention this context in my mail because the error noted in
those user reports is surprisingly a bit different than what I have
seen in my experiments on Windows and I don't think I have so far
narrowed it down to a JDK issue (if any). In their case, it appears
the call went past the issue we are discussing this mail and instead
failed later with something like:
java.nio.file.FileSystemException: E:\nul: Incorrect function.
at
sun.nio.fs.WindowsException.translateToIOException(WindowsException.java:86)
at
sun.nio.fs.WindowsException.rethrowAsIOException(WindowsException.java:97)
at
sun.nio.fs.WindowsException.rethrowAsIOException(WindowsException.java:102)
at
sun.nio.fs.WindowsFileSystemProvider.newByteChannel(WindowsFileSystemProvider.java:230)
at
java.nio.file.spi.FileSystemProvider.newOutputStream(FileSystemProvider.java:434)
at java.nio.file.Files.newOutputStream(Files.java:216)
I'm not yet sure how they managed to get to that stage and am waiting
to see if they can provide us with a reproducible build file to
reproduce this.
FWIW - that bug report states that they ran into this even when using
"nul" and not just "nul:". So there might be something more going on
here and am just waiting to see if they can provide us a build file to
reproduce this issue.
-Jaikiran
Re: java.io.File#toPath() on Windows doesn't understand "NUL:" (null device)?
Hello Alan,
On 17/03/21 2:45 pm, Alan Bateman wrote:
On 17/03/2021 08:21, Jaikiran Pai wrote:
:
I can confirm that using "NUL" or "nul" work fine in the above code,
I don't know the context for your question
A while back Apache Ant switched to using the
Files.newInputStream/Files.newOutputStream construct as a replacement
for the FileInputStream and FileOutputStream constructors[1]. That
commit seems to have introduced an regression in Ant as noted here[2].
It appears that users of Ant were using "nul" (and even "nul:") as
destination file to have Ant write the data to that destination.
Internally Ant constructs an instance of java.io.File from the user
provided path string ("nul" or "nul:" in this case) and constructs a
OutputStream out of it. Previously (before we made that commit in Ant),
it would be using the FileOutputStream constructor (and would succeed)
and now it uses the Files.newOuputStream(...) which expects a Path
instance and this where our usage of java.io.File.toPath() ran into the
issue I note in this mail, when I started investigating this.
I didn't mention this context in my mail because the error noted in
those user reports is surprisingly a bit different than what I have seen
in my experiments on Windows and I don't think I have so far narrowed it
down to a JDK issue (if any). In their case, it appears the call went
past the issue we are discussing this mail and instead failed later with
something like:
java.nio.file.FileSystemException: E:\nul: Incorrect function.
at
sun.nio.fs.WindowsException.translateToIOException(WindowsException.java:86)
at
sun.nio.fs.WindowsException.rethrowAsIOException(WindowsException.java:97)
at
sun.nio.fs.WindowsException.rethrowAsIOException(WindowsException.java:102)
at
sun.nio.fs.WindowsFileSystemProvider.newByteChannel(WindowsFileSystemProvider.java:230)
at
java.nio.file.spi.FileSystemProvider.newOutputStream(FileSystemProvider.java:434)
at java.nio.file.Files.newOutputStream(Files.java:216)
I'm not yet sure how they managed to get to that stage and am waiting to
see if they can provide us with a reproducible build file to reproduce this.
but just to mention InputStream.nullInputStream() or
Reader.nullReader() may be useful if you have something that wants to
read from a null stream.
We have had a recent discussion in Ant dev mailing list[3] to introduce
such a construct in some of our Ant tasks where users can tell us that
they want the output/error output discarded (that's what they are using
/dev/null and "nul:" for). That will prevent all these platform specific
usages of null device representations. Internally, in the
implementation, we will use the APIs like the one you note and avoid
having to deal with null devices.
[1]
https://github.com/apache/ant/commit/af74d1f6b882cef5f4167d972638ad886d12d58c
[2] https://bz.apache.org/bugzilla/show_bug.cgi?id=62330
[3] https://www.mail-archive.com/dev@ant.apache.org/msg48730.html
-Jaikiran
Re: java.io.File#toPath() on Windows doesn't understand "NUL:" (null device)?
On 17/03/2021 08:21, Jaikiran Pai wrote: : I can confirm that using "NUL" or "nul" work fine in the above code, I don't know the context for your question but just to mention InputStream.nullInputStream() or Reader.nullReader() may be useful if you have something that wants to read from a null stream. -Alan.
Re: RFR: 8261785: Calling "main" method in anonymous nested class crashes the JVM [v3]
On Tue, 16 Mar 2021 17:49:42 GMT, Henry Jen wrote: > > Using an anonymous class for the main class looks strange and hard to > > believe anyone is relying on this. I wonder if we should do more checking > > LauncherHelper.validateMainClass to reject cases like this. > > I raised that same question, and people tends to agree launcher could reject > anonymous/local classes. But pointed out that should require a CSR review. > Therefore I chose to fix crash first, and we can file another ticket to > address main class requirements. The requirement for a CSR and release note should not be a concern here. I don't object to the fix but I think it would be very useful to document the main class and whether local or anonymous classes can be used, its accessibility, and the accessibility of the main method. We had to do something similar recently with the premain method (java.lang.instrument). - PR: https://git.openjdk.java.net/jdk/pull/2999
Re: java.io.File#toPath() on Windows doesn't understand "NUL:" (null device)?
On 17/03/21 1:26 pm, Alan Bateman wrote: On 17/03/2021 03:21, Jaikiran Pai wrote: : The code tries to read from NUL: on a Windows setup. This code runs into the following exception on Windows when the java.io.File#toPath() gets invoked: Exception in thread "main" java.nio.file.InvalidPathException: Illegal char <:> at index 3: NUL: at java.base/sun.nio.fs.WindowsPathParser.normalize(WindowsPathParser.java:182) at java.base/sun.nio.fs.WindowsPathParser.parse(WindowsPathParser.java:153) at java.base/sun.nio.fs.WindowsPathParser.parse(WindowsPathParser.java:77) at java.base/sun.nio.fs.WindowsPath.parse(WindowsPath.java:92) at java.base/sun.nio.fs.WindowsFileSystem.getPath(WindowsFileSystem.java:230) at java.base/java.io.File.toPath(File.java:2316) at FileTest.main(FileTest.java:18) So it looks like java.io.File.toPath() on Windows isn't able to recognize the null device construct? Special devices, esp. those historical devices from the DOS era, are very problematic. NUL is somewhat benign compared to the other and you use "NUL" (not "NUL:") then should work as you expect. Thank you David and Alan. I can confirm that using "NUL" or "nul" work fine in the above code, with the FileInputStream/FileOutputStream constructors as well as Files.newInputStream(f.toPath()) and Files.newOutputStream(f.toPath()). Changing the path parser to allow ":" in places other than after drive letters is a slippery slope as it brings all a lot of the issues that plagued the older code. Understood. -Jaikiran
Re: java.io.File#toPath() on Windows doesn't understand "NUL:" (null device)?
On 17/03/2021 03:21, Jaikiran Pai wrote: : The code tries to read from NUL: on a Windows setup. This code runs into the following exception on Windows when the java.io.File#toPath() gets invoked: Exception in thread "main" java.nio.file.InvalidPathException: Illegal char <:> at index 3: NUL: at java.base/sun.nio.fs.WindowsPathParser.normalize(WindowsPathParser.java:182) at java.base/sun.nio.fs.WindowsPathParser.parse(WindowsPathParser.java:153) at java.base/sun.nio.fs.WindowsPathParser.parse(WindowsPathParser.java:77) at java.base/sun.nio.fs.WindowsPath.parse(WindowsPath.java:92) at java.base/sun.nio.fs.WindowsFileSystem.getPath(WindowsFileSystem.java:230) at java.base/java.io.File.toPath(File.java:2316) at FileTest.main(FileTest.java:18) So it looks like java.io.File.toPath() on Windows isn't able to recognize the null device construct? Special devices, esp. those historical devices from the DOS era, are very problematic. NUL is somewhat benign compared to the other and you use "NUL" (not "NUL:") then should work as you expect. Changing the path parser to allow ":" in places other than after drive letters is a slippery slope as it brings all a lot of the issues that plagued the older code. -Alan
Re: java.io.File#toPath() on Windows doesn't understand "NUL:" (null device)?
https://bugs.openjdk.java.net/browse/JDK-8022671
Cheers,
David
On 17/03/2021 1:21 pm, Jaikiran Pai wrote:
Please consider this trivial code:
import java.io.*;
import java.nio.file.*;
public class FileTest {
public static void main(final String[] args) throws Exception {
System.getProperties().list(System.out);
final File f = new File("NUL:");
try (final InputStream fis = Files.newInputStream(f.toPath())) {
int numBytes = 0;
while (fis.read() != -1) {
System.out.println("Files.newInputStream - Read a
byte from " + f);
numBytes++;
}
System.out.println("Files.newInputStream - Done reading
" + numBytes + " bytes from " + f);
}
}
}
The code tries to read from NUL: on a Windows setup. This code runs into
the following exception on Windows when the java.io.File#toPath() gets
invoked:
Exception in thread "main" java.nio.file.InvalidPathException: Illegal
char <:> at index 3: NUL:
at
java.base/sun.nio.fs.WindowsPathParser.normalize(WindowsPathParser.java:182)
at
java.base/sun.nio.fs.WindowsPathParser.parse(WindowsPathParser.java:153)
at
java.base/sun.nio.fs.WindowsPathParser.parse(WindowsPathParser.java:77)
at java.base/sun.nio.fs.WindowsPath.parse(WindowsPath.java:92)
at
java.base/sun.nio.fs.WindowsFileSystem.getPath(WindowsFileSystem.java:230)
at java.base/java.io.File.toPath(File.java:2316)
at FileTest.main(FileTest.java:18)
So it looks like java.io.File.toPath() on Windows isn't able to
recognize the null device construct?
Just to make sure this issue resides only this specific API
implementation, I switched the above code to use new FileInputStream(f)
as follows and that works as expected - reads 0 bytes and completes
successfully. So the NUL: construct is indeed understood by the other APIs.
public class FileTest {
public static void main(final String[] args) throws Exception {
System.getProperties().list(System.out);
final File f = new File("NUL:");
try (final FileInputStream fis = new FileInputStream(f)) {
int numBytes = 0;
while (fis.read() != -1) {
System.out.println("FileInputStream() - Read a byte
from " + f);
numBytes++;
}
System.out.println("FileInputStream() - Done reading "
+ numBytes + " bytes from " + f);
}
}
}
Output:
FileInputStream() - Done reading 0 bytes from NUL:
Test results are from latest Java 16 release on a Windows setup.
-Jaikiran
