Re: RFR 8207750 : Native handle leak in java.io.WinNTFileSystem.list()
Hello, I am not sure about the JNi conventions, but does the jObjectArray rv need to be released as local reference in some of the early returns as well? Gruss Bernd Gruss Bernd -- http://bernd.eckenfels.net Von: -1031249216m Auftrag von Gesendet: Mittwoch, Juli 18, 2018 4:38 AM An: Ivan Gerasimov Cc: core-libs-dev Betreff: Re: RFR 8207750 : Native handle leak in java.io.WinNTFileSystem.list() Hi Ivan, This looks all right to me. Thanks, Brian On Jul 17, 2018, at 5:01 PM, Ivan Gerasimov wrote: > In the file WinNTFileSystem_md.c there is a potential leak of native handle > obtained from FindFirstFileW() in the unfortunate event of memory allocation > failure. > > Would you please help review a simple fix? > > BUGURL: https://bugs.openjdk.java.net/browse/JDK-8207750 > WEBREV:http://cr.openjdk.java.net/~igerasim/8207750/00/webrev/
any movement on fixing the breakage of logback MDC with CompletableFutures?
I wanted to revisit the fact that MDC of logback broke in java's CompletableFuture while it works just fine with scala twitter Future because of their Local.scala class that acts like a ThreadLocal BUT for Futures transferring context. There is basically no way to fix it except by threading in some sort of LocalFuture.java into the CompletableFutures which would rock. thanks for any info there, (I would rather not switch this whole project to scala and having the MDC in logback broken is extremely annoying) https://stackoverflow.com/questions/37933713/does-completablefuture-have-a-corresponding-local-context thanks Dean
RFR 8207753 : Handle to process snapshot is leaked if Process32First() fails
Hello! In native functions java.lang.ProcessHandleImpl.getProcessPids0() and java.lang.ProcessHandleImpl.parent0() a handle to a snapshot of all processes in the system is obtained with CreateToolhelp32Snapshot(). This handle isn't releases if the subsequent call to Process32First() fails. Would you please help review a trivial fix? BUGURL: https://bugs.openjdk.java.net/browse/JDK-8207753 WEBREV: http://cr.openjdk.java.net/~igerasim/8207753/00/webrev/ Thanks in advance! -- With kind regards, Ivan Gerasimov
Re: [RFC] StringBuilder.toCharArray
You can already copy a StringBuilder's contents into a String or another StringBuilder. What would Josh say? "Doesn't pull its weight." ?! On Wed, Jul 18, 2018 at 7:29 AM, Isaac Levy wrote: > Is there any interest in a toCharArray method for StringBuilder? I'm > unable to to make a bug for it. > > Just a bit of sugar: > > char[] toCharArray() { > int length = length(); > char[] dst = new char[length]; > getChars(0, length, dst, 0); > return dst; > } > > Regards, > Isaac >
Re: RFR : 8207395: jar has issues with UNC-path arguments for the jar -C parameter [windows]
Ideally it would be preferred to move jar to a complete nio implementation and we then can leave the WindowsPath to take care of this stuff. That said that would be a relative big change. I do have a version on my disk but need clean up, test, performance measurement and review to get it. So for now I think it's fine to add this small fix to support the unc. now sure if the following is better? 637 boolean hasUNC = File.separatorChar == '\\'&& dir.startsWith("//"); 638 while (dir.indexOf("//")> -1) { 639 dir = dir.replace("//", "/"); 640 } 641 if (isUNC) // Restore Windows UNC path. 642 dir = "/" + dir; -Sherman On 7/18/18, 6:44 AM, Roger Riggs wrote: Hi, Is there any chance that just using java.nio.file.Path.of will do the needed cleanup? (or Paths.get) It seems a shame to spread this kind of adhoc fixup around? $0.02, Roger On 7/18/2018 5:31 AM, Lindenmaier, Goetz wrote: Hi Matthias, thanks for doing this fix. I think this can be noted down a bit better, avoiding duplicating the loop. Also, please remove the redundant dir.replace(File.separatorChar, '/'). dir = dir.replace(File.separatorChar, '/'); +String unc = (dir.startsWith("//") && (File.separatorChar == '\\')) ? "/" : ""; while (dir.indexOf("//") > -1) { dir = dir.replace("//", "/"); } - pathsMap.get(version).add(dir.replace(File.separatorChar, '/')); +// Restore the second leading '/' needed for the Windows UNC path. +dir = unc + dir; +pathsMap.get(version).add(dir); nameBuf[k++] = dir + args[++i]; Best regards, Goetz. -Original Message- From: core-libs-dev [mailto:core-libs-dev-boun...@openjdk.java.net] On Behalf Of Baesken, Matthias Sent: Dienstag, 17. Juli 2018 13:15 To: core-libs-dev@openjdk.java.net Subject: [CAUTION] RFR : 8207395: jar has issues with UNC-path arguments for the jar -C parameter [windows] Please review this small fix for allowing windows UNC paths in the jar -C parameter. Currently passing a UNC path to a directory with -Cfails : c:\testdir>c:\tools\jdk10\bin\jar.exe test.jar -C \\MYMACHINE\subdir README.txt Illegal option: s Try `jar --help' for more information. With the patch it works . webrev http://cr.openjdk.java.net/~mbaesken/webrevs/8207395/ bug https://bugs.openjdk.java.net/browse/JDK-8207395 Thanks, Matthias
Re: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
On 18/07/2018 09:21, Baesken, Matthias wrote: Hi Alan, I'll prepare a CSR . I selected a more general name "jdk.includeInExceptions" , because there is the idea to enhance more exceptions with additional output . In such a case " jdk.util.jar.includeInExceptions" would not really help . However so far it is still a bit unclear how many exceptions we would like to enhance , so this has to be checked first . jdk.includeInExceptions expands the scope. That might be okay but we will need to re-visit jdk.net.includeInExceptions and also move the support to somewhere in jdk.internal so that it can be used by other code in java.base. -Alan
RE: RFR : 8207395: jar has issues with UNC-path arguments for the jar -C parameter [windows]
Good point, I think I better keep the "good old" loop . Second webrev containing the suggestions of Goetz : http://cr.openjdk.java.net/~mbaesken/webrevs/8207395.1/ Best regards, Matthias > -Original Message- > From: Scott Palmer [mailto:swpal...@gmail.com] > Sent: Mittwoch, 18. Juli 2018 15:05 > To: Baesken, Matthias > Cc: Lindenmaier, Goetz ; core-libs- > d...@openjdk.java.net > Subject: Re: RFR : 8207395: jar has issues with UNC-path arguments for the > jar -C parameter [windows] > > That gives a different result. > > Original: > "///" -> “/" > > replaceAll: > "///" -> “//" > > > > On Jul 18, 2018, at 7:18 AM, Baesken, Matthias > wrote: > > > > Hi Götz, thanks for the input ! > > Should we maybe use > > > > dir.replaceAll > > > > and not the while loop ? > > > >> while (dir.indexOf("//") > -1) { > >> dir = dir.replace("//", "/"); > >> } > > > > Best regards, Matthias > > > > > >> -Original Message- > >> From: Lindenmaier, Goetz > >> Sent: Mittwoch, 18. Juli 2018 11:32 > >> To: Baesken, Matthias ; core-libs- > >> d...@openjdk.java.net > >> Subject: RE: RFR : 8207395: jar has issues with UNC-path arguments for > the > >> jar -C parameter [windows] > >> > >> Hi Matthias, > >> > >> thanks for doing this fix. > >> > >> I think this can be noted down a bit better, avoiding duplicating the loop. > >> Also, please remove the redundant dir.replace(File.separatorChar, '/'). > >> > >> dir = dir.replace(File.separatorChar, '/'); > >> +String unc = (dir.startsWith("//") && > >> (File.separatorChar == > '\\')) > >> ? "/" : ""; > >> while (dir.indexOf("//") > -1) { > >> dir = dir.replace("//", "/"); > >> } > >> - > >> pathsMap.get(version).add(dir.replace(File.separatorChar, > '/')); > >> +// Restore the second leading '/' needed for the > >> Windows > UNC > >> path. > >> +dir = unc + dir; > >> +pathsMap.get(version).add(dir); > >> nameBuf[k++] = dir + args[++i]; > >> > >> Best regards, > >> Goetz. > >> > >>> -Original Message- > >>> From: core-libs-dev [mailto:core-libs-dev-boun...@openjdk.java.net] > On > >>> Behalf Of Baesken, Matthias > >>> Sent: Dienstag, 17. Juli 2018 13:15 > >>> To: core-libs-dev@openjdk.java.net > >>> Subject: [CAUTION] RFR : 8207395: jar has issues with UNC-path > arguments > >>> for the jar -C parameter [windows] > >>> > >>> Please review this small fix for allowing windows UNC paths in the > >>> jar - > C > >>> parameter. > >>> Currently passing a UNC path to a directory with -Cfails : > >>> > >>> c:\testdir>c:\tools\jdk10\bin\jar.exe test.jar -C \\MYMACHINE\subdir > >>> README.txt > >>> Illegal option: s > >>> Try `jar --help' for more information. > >>> > >>> With the patch it works . > >>> > >>> webrev > >>> > >>> http://cr.openjdk.java.net/~mbaesken/webrevs/8207395/ > >>> > >>> bug > >>> > >>> https://bugs.openjdk.java.net/browse/JDK-8207395 > >>> > >>> > >>> > >>> > >>> Thanks, Matthias > >
Re: [PATCH] AbstractStringBuilder.append()
I think adding a dedicated method would help clarify and encourage performant code. For example, I sped up the snippet below after seeing that StringBuilder.append() has a fast path when the arg is another StringBuilder, which isn't clear from the javadoc. public class ScalaSB implements java.lang.CharSequence { final StringBuilder sb; ... public append(ScalaSB other) { - sb.append(other); +sb.append(other.sb); } }
[RFC] StringBuilder.toCharArray
Is there any interest in a toCharArray method for StringBuilder? I'm unable to to make a bug for it. Just a bit of sugar: char[] toCharArray() { int length = length(); char[] dst = new char[length]; getChars(0, length, dst, 0); return dst; } Regards, Isaac
Re: [PATCH] regex matcher opt: remove redundant StringBuilder
I still think it would be valuable to land a patch for replaceAll to avoid temporary StringBuilders, is there anyone who wants to help me land it? Isaac On Sun, Apr 29, 2018 at 10:24 PM, Isaac Levy wrote: > Your patch looks good to me, and will get the majority of performance > benefits without any API issues. > > Isaac > > > On Tue, Apr 24, 2018 at 9:38 PM, Xueming Shen wrote: >> for String based replaceAll/First() it might be worth doing something like >> >> http://cr.openjdk.java.net/~sherman/regex_removesb/webrev/ >> >> >> On 4/24/18, 10:47 AM, Isaac Levy wrote: >>> >>> Hi Sherman, >>> >>> Thanks for clarifying. Looks like exceptions are caused by invalid >>> format string. I wouldn't expect most programs to be catching this and >>> preserving their buffer, but dunno. >>> >>> How much does it affect perf? Well it depends on use case, a jmh of >>> replaceAll with a length 200 string of digits and regex "(\w)" shows >>> about 30% speedup. >>> >>> [info] Benchmark Mode Cnt Score Error Units >>> [info] origM avgt 10 11.669 ± 0.211 us/op >>> [info] newM avgt 10 8.926 ± 0.095 us/op >>> >>> Isaac >>> >>> >>> On Tue, Apr 24, 2018 at 12:53 PM, Xueming Shen >>> wrote: Hi Isaac, I actually meant to say "we are not supposed to output the partial text into the output buffer in case of an exception". It has nothing to do with the changeset you cited. This has been the behavior since day one/JDK1.4, though it is not specified explicitly in the API doc. The newly added StringBuilder variant simply follows this behavior. If it's really desired it is kinda doable to save that StringBuilder without the "incompatible" behavior change but just wonder if it is really worth the effort. Thanks, Sherman On 4/24/18, 9:11 AM, Isaac Levy wrote: > > (moving this to a separate discussion) > > > --- a/src/java.base/share/classes/java/util/regex/Matcher.java > +++ b/src/java.base/share/classes/java/util/regex/Matcher.java > @@ -993,13 +993,11 @@ > public Matcher appendReplacement(StringBuilder sb, String > replacement) { >// If no match, return error >if (first< 0) >throw new IllegalStateException("No match available"); > -StringBuilder result = new StringBuilder(); > -appendExpandedReplacement(replacement, result); >// Append the intervening text >sb.append(text, lastAppendPosition, first); >// Append the match substitution > +appendExpandedReplacement(replacement, sb); > -sb.append(result); > > > > On Mon, Apr 23, 2018 at 5:05 PM Xueming Shen > wrote: >> >> >> I would assume in case of an exception thrown from >> appendExpandedReplacement() we don't >> want "text" to be pushed into the "sb". >> >> -sherman > > > Perhaps. Though the behavior under exception is undefined and this > function is probably primarily used though the replaceAll API, which > wouldn’t return the intermediate sb under the case of exception. > > My reading of the blame was the temp StringBuilder was an artifact of > the api previously using StringBuffer externally. See > http://hg.openjdk.java.net/jdk9/dev/jdk/rev/763c564451b3 >>
Re: RFR : 8207395: jar has issues with UNC-path arguments for the jar -C parameter [windows]
Hi, Is there any chance that just using java.nio.file.Path.of will do the needed cleanup? (or Paths.get) It seems a shame to spread this kind of adhoc fixup around? $0.02, Roger On 7/18/2018 5:31 AM, Lindenmaier, Goetz wrote: Hi Matthias, thanks for doing this fix. I think this can be noted down a bit better, avoiding duplicating the loop. Also, please remove the redundant dir.replace(File.separatorChar, '/'). dir = dir.replace(File.separatorChar, '/'); +String unc = (dir.startsWith("//") && (File.separatorChar == '\\')) ? "/" : ""; while (dir.indexOf("//") > -1) { dir = dir.replace("//", "/"); } - pathsMap.get(version).add(dir.replace(File.separatorChar, '/')); +// Restore the second leading '/' needed for the Windows UNC path. +dir = unc + dir; +pathsMap.get(version).add(dir); nameBuf[k++] = dir + args[++i]; Best regards, Goetz. -Original Message- From: core-libs-dev [mailto:core-libs-dev-boun...@openjdk.java.net] On Behalf Of Baesken, Matthias Sent: Dienstag, 17. Juli 2018 13:15 To: core-libs-dev@openjdk.java.net Subject: [CAUTION] RFR : 8207395: jar has issues with UNC-path arguments for the jar -C parameter [windows] Please review this small fix for allowing windows UNC paths in the jar -C parameter. Currently passing a UNC path to a directory with -Cfails : c:\testdir>c:\tools\jdk10\bin\jar.exe test.jar -C \\MYMACHINE\subdir README.txt Illegal option: s Try `jar --help' for more information. With the patch it works . webrev http://cr.openjdk.java.net/~mbaesken/webrevs/8207395/ bug https://bugs.openjdk.java.net/browse/JDK-8207395 Thanks, Matthias
Re: RFR : 8207395: jar has issues with UNC-path arguments for the jar -C parameter [windows]
That gives a different result. Original: "///" -> “/" replaceAll: "///" -> “//" > On Jul 18, 2018, at 7:18 AM, Baesken, Matthias > wrote: > > Hi Götz, thanks for the input ! > Should we maybe use > > dir.replaceAll > > and not the while loop ? > >> while (dir.indexOf("//") > -1) { >> dir = dir.replace("//", "/"); >> } > > Best regards, Matthias > > >> -Original Message- >> From: Lindenmaier, Goetz >> Sent: Mittwoch, 18. Juli 2018 11:32 >> To: Baesken, Matthias ; core-libs- >> d...@openjdk.java.net >> Subject: RE: RFR : 8207395: jar has issues with UNC-path arguments for the >> jar -C parameter [windows] >> >> Hi Matthias, >> >> thanks for doing this fix. >> >> I think this can be noted down a bit better, avoiding duplicating the loop. >> Also, please remove the redundant dir.replace(File.separatorChar, '/'). >> >> dir = dir.replace(File.separatorChar, '/'); >> +String unc = (dir.startsWith("//") && >> (File.separatorChar == '\\')) >> ? "/" : ""; >> while (dir.indexOf("//") > -1) { >> dir = dir.replace("//", "/"); >> } >> - >> pathsMap.get(version).add(dir.replace(File.separatorChar, '/')); >> +// Restore the second leading '/' needed for the >> Windows UNC >> path. >> +dir = unc + dir; >> +pathsMap.get(version).add(dir); >> nameBuf[k++] = dir + args[++i]; >> >> Best regards, >> Goetz. >> >>> -Original Message- >>> From: core-libs-dev [mailto:core-libs-dev-boun...@openjdk.java.net] On >>> Behalf Of Baesken, Matthias >>> Sent: Dienstag, 17. Juli 2018 13:15 >>> To: core-libs-dev@openjdk.java.net >>> Subject: [CAUTION] RFR : 8207395: jar has issues with UNC-path arguments >>> for the jar -C parameter [windows] >>> >>> Please review this small fix for allowing windows UNC paths in the jar >>> -C >>> parameter. >>> Currently passing a UNC path to a directory with -Cfails : >>> >>> c:\testdir>c:\tools\jdk10\bin\jar.exe test.jar -C \\MYMACHINE\subdir >>> README.txt >>> Illegal option: s >>> Try `jar --help' for more information. >>> >>> With the patch it works . >>> >>> webrev >>> >>> http://cr.openjdk.java.net/~mbaesken/webrevs/8207395/ >>> >>> bug >>> >>> https://bugs.openjdk.java.net/browse/JDK-8207395 >>> >>> >>> >>> >>> Thanks, Matthias >
RE: RFR : 8207395: jar has issues with UNC-path arguments for the jar -C parameter [windows]
Hi Götz, thanks for the input ! Should we maybe use dir.replaceAll and not the while loop ? > while (dir.indexOf("//") > -1) { > dir = dir.replace("//", "/"); > } Best regards, Matthias > -Original Message- > From: Lindenmaier, Goetz > Sent: Mittwoch, 18. Juli 2018 11:32 > To: Baesken, Matthias ; core-libs- > d...@openjdk.java.net > Subject: RE: RFR : 8207395: jar has issues with UNC-path arguments for the > jar -C parameter [windows] > > Hi Matthias, > > thanks for doing this fix. > > I think this can be noted down a bit better, avoiding duplicating the loop. > Also, please remove the redundant dir.replace(File.separatorChar, '/'). > > dir = dir.replace(File.separatorChar, '/'); > +String unc = (dir.startsWith("//") && > (File.separatorChar == '\\')) > ? "/" : ""; > while (dir.indexOf("//") > -1) { > dir = dir.replace("//", "/"); > } > - > pathsMap.get(version).add(dir.replace(File.separatorChar, '/')); > +// Restore the second leading '/' needed for the > Windows UNC > path. > +dir = unc + dir; > +pathsMap.get(version).add(dir); > nameBuf[k++] = dir + args[++i]; > > Best regards, > Goetz. > > > -Original Message- > > From: core-libs-dev [mailto:core-libs-dev-boun...@openjdk.java.net] On > > Behalf Of Baesken, Matthias > > Sent: Dienstag, 17. Juli 2018 13:15 > > To: core-libs-dev@openjdk.java.net > > Subject: [CAUTION] RFR : 8207395: jar has issues with UNC-path arguments > > for the jar -C parameter [windows] > > > > Please review this small fix for allowing windows UNC paths in the jar > > -C > > parameter. > > Currently passing a UNC path to a directory with -Cfails : > > > > c:\testdir>c:\tools\jdk10\bin\jar.exe test.jar -C \\MYMACHINE\subdir > > README.txt > > Illegal option: s > > Try `jar --help' for more information. > > > > With the patch it works . > > > > webrev > > > > http://cr.openjdk.java.net/~mbaesken/webrevs/8207395/ > > > > bug > > > > https://bugs.openjdk.java.net/browse/JDK-8207395 > > > > > > > > > > Thanks, Matthias
RE: RFR : 8207395: jar has issues with UNC-path arguments for the jar -C parameter [windows]
Hi Matthias, thanks for doing this fix. I think this can be noted down a bit better, avoiding duplicating the loop. Also, please remove the redundant dir.replace(File.separatorChar, '/'). dir = dir.replace(File.separatorChar, '/'); +String unc = (dir.startsWith("//") && (File.separatorChar == '\\')) ? "/" : ""; while (dir.indexOf("//") > -1) { dir = dir.replace("//", "/"); } - pathsMap.get(version).add(dir.replace(File.separatorChar, '/')); +// Restore the second leading '/' needed for the Windows UNC path. +dir = unc + dir; +pathsMap.get(version).add(dir); nameBuf[k++] = dir + args[++i]; Best regards, Goetz. > -Original Message- > From: core-libs-dev [mailto:core-libs-dev-boun...@openjdk.java.net] On > Behalf Of Baesken, Matthias > Sent: Dienstag, 17. Juli 2018 13:15 > To: core-libs-dev@openjdk.java.net > Subject: [CAUTION] RFR : 8207395: jar has issues with UNC-path arguments > for the jar -C parameter [windows] > > Please review this small fix for allowing windows UNC paths in the jar -C > parameter. > Currently passing a UNC path to a directory with -Cfails : > > c:\testdir>c:\tools\jdk10\bin\jar.exe test.jar -C \\MYMACHINE\subdir > README.txt > Illegal option: s > Try `jar --help' for more information. > > With the patch it works . > > webrev > > http://cr.openjdk.java.net/~mbaesken/webrevs/8207395/ > > bug > > https://bugs.openjdk.java.net/browse/JDK-8207395 > > > > > Thanks, Matthias
RE: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
> The name of the security/system property will need discussion as > "jdk.includeInExceptions" is very general. If we have something general > then we'll need a good name and replace the existing > jdk.net.includeInExceptions. It might be better to go with something > specific for the area such as "jdk.util.jar.includeInExceptions=jarpath" > (to be consistent with other jdk.* properties in this code). A CSR will > be needed for this too as the property will be documented in the > java.security file. Hi Alan, I'll prepare a CSR . I selected a more general name "jdk.includeInExceptions" , because there is the idea to enhance more exceptions with additional output . In such a case " jdk.util.jar.includeInExceptions" would not really help . However so far it is still a bit unclear how many exceptions we would like to enhance , so this has to be checked first . Best regards, Matthias > -Original Message- > From: Alan Bateman [mailto:alan.bate...@oracle.com] > Sent: Dienstag, 17. Juli 2018 13:39 > To: Baesken, Matthias ; core-libs- > d...@openjdk.java.net > Subject: Re: [RFR] 8205525 : Improve exception messages during manifest > parsing of jar archives > > On 16/07/2018 14:53, Baesken, Matthias wrote: > > Hello, after latest comments from Alan and JaikiranI created a new > webrev : > > > > http://cr.openjdk.java.net/~mbaesken/webrevs/8205525.2/ > > > > The jar file path is now printed in case jdk.includeInExceptions > > contains > jarpath (this approach is "borrowed" from the enhanced socket > exceptions ) . > > The line number is always printed . > > > The general approach seems okay and consistent with the agreement on > how > to reveal host names in socket exceptions. > > The name of the security/system property will need discussion as > "jdk.includeInExceptions" is very general. If we have something general > then we'll need a good name and replace the existing > jdk.net.includeInExceptions. It might be better to go with something > specific for the area such as "jdk.util.jar.includeInExceptions=jarpath" > (to be consistent with other jdk.* properties in this code). A CSR will > be needed for this too as the property will be documented in the > java.security file. > > As regards the patch then it mostly looks okay but I think the changes > in Attributes will need cleanup to get it consistent (esp. the line > lengths) with the existing code. > > -Alan
RFR(S): 8207766: [testbug] Adapt tests for Aix.
Hi, I would like to fix these tests to run on aix: http://cr.openjdk.java.net/~goetz/wr18/8207766-aixTestFixes/01/ MulticastSocket tests Opened bug and asked IBM to fix. Put on ProblemList. See https://bugs.openjdk.java.net/browse/JDK-8207404 EvalArraysAsList.sh Quotes need to be escaped on AIX. WrappedToolkitTest.sh Check for AIX missing. PKCS11Test.java getNSSLibDir can return null. If so, static initialization of badNSSVersion fails: java.io.FileNotFoundException: nulllibsoftokn3.so (No such file or directory) TestNssDbSqlite.java Don't run this on AIX. There is no NSS on basic installations. launcher/SourceMode.java AIX does not support arguments to the shebang. Best regards, Goetz.