September 2018 - core-libs-dev

RFR: 8203629: Produce events in the JDK without a dependency on jdk.jfr
by Erik Gahlin 30 Oct '18

30 Oct '18

Hi, Could I have a review of an enhancement that will make it possible to add JFR events to java.base, and possibly other modules in the JDK, without a compile time dependency on jdk.jfr. Bug: https://bugs.openjdk.java.net/browse/JDK-8203629 Webrev: http://cr.openjdk.java.net/~egahlin/8203629.0 Testing: Tests in test/jdk/jdk/jfr The functionality is a prerequisite for an upcoming enhancement that will add JFR events to the security libraries. In short, jdk.jfr.Event (located in jdk.jfr) extends jdk.internal.event.Event (located in java.base). Metadata for events, such as labels and descriptions, are added using a mirror event class located in jdk.jfr module. If the fields of the mirror class and the event class don't match an InternalError is thrown. To illustrate how the mechanism can be used, see the following example (not meant to be checked in). http://cr.openjdk.java.net/~egahlin/8203629.example Since the implementation of jdk.internal.event.Event is empty, the JIT will typically eliminate all traces of JFR functionality unless Flight Recorder is enabled. Thanks Erik

4 11

RFR: 8139965 - Hang seen when using com.sun.jndi.ldap.search.replyQueueSize
by Rob McKenna 26 Oct '18

26 Oct '18

Hi folks, I'd like to get a re-review of this change: https://bugs.openjdk.java.net/browse/JDK-8139965 http://cr.openjdk.java.net/~robm/8139965/webrev/ In case the original has gone stale: http://mail.openjdk.java.net/pipermail/core-libs-dev/2016-August/042767.html -Rob

4 10

RFR(s): 8210549: Runtime.exec: in closeDescriptors(), use FD_CLOEXEC instead of close()
by Thomas Stüfe 24 Oct '18

24 Oct '18

Hi all, May I please have reviews for this small fix: Bug: https://bugs.openjdk.java.net/browse/JDK-8210549 patch (full): http://cr.openjdk.java.net/~stuefe/webrevs/8210549-Use-FD_CLOEXEC-instead-o… patch (minimal alternative): http://cr.openjdk.java.net/~stuefe/webrevs/8210549-Use-FD_CLOEXEC-instead-o… See also prior discussion: http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-September/055173.… Submit tests ran with full patch successfully through. --- Background: After fork()/vfork() and before exec(), the child process needs to close all inherited file descriptors apart from the stdin/out/err pipe ends. We do this by iterating thru all file descriptors in /proc/<pid>/fd or whatever the equivalent is on that platform. This is done using opendir(), readdir(). We then close all these file descriptors using close(). The problem with that technique is that we may accidentally close the file descriptor opendir() is using internally to read the directory content of /proc/<pid>/fd, thereby sawing off the branch we are sitting on. The coding does some magic to attempt to prevent this: <quote> 86 /* We're trying to close all file descriptors, but opendir() might 87 * itself be implemented using a file descriptor, and we certainly 88 * don't want to close that while it's in use. We assume that if 89 * opendir() is implemented using a file descriptor, then it uses 90 * the lowest numbered file descriptor, just like open(). So we 91 * close a couple explicitly. */ 92 93 close(from_fd); /* for possible use by opendir() */ 94 close(from_fd + 1); /* another one for good luck */ ... 108 while ((dirp = readdir64(dp)) != NULL) { 109 int fd; 110 if (isAsciiDigit(dirp->d_name[0]) && 111 (fd = strtol(dirp->d_name, NULL, 10)) >= from_fd + 2)Improve "close all filedescriptors" coding. 112 close(fd); 113 } </quote> This workaround can be removed if, instead of outright closing the file descriptor in the loop, we were to set the file descriptor to FD_CLOEXEC. Closing all descriptors would be defered to the actual exec() call some milliseconds later. ---- The patch changes the close() call to fcntl(FD_CLOEXEC). This removes the need for the workaround as described. In addition to that, the full version of the patch also adds error handling to the readdir() loop. But note that this exposes us to a behavioral change should we run into a readdir() error: - currently, we would leave the remaining file descriptors quietly unclosed. This usually would have no effect, but in rare cases may lead to difficult-to-analyze errors when child processes accidentally disturb parent process IO. - With this patch, we will fail if readdir fails. However, we do not just yet fail the Runtime.exec() call, but enter a fallback code section here: 364 /* close everything */ 365 if (closeDescriptors() == 0) { /* failed, close the old way */ 366 int max_fd = (int)sysconf(_SC_OPEN_MAX); 367 int fd; 368 for (fd = FAIL_FILENO + 1; fd < max_fd; fd++) 369 if (close(fd) == -1 && errno != EBADF) 370 goto WhyCantJohnnyExec; 371 } I am not convinced this is a good fallback. _SC_OPEN_MAX depends on ulimit -Hn and may be high. On my Ubuntu 16.4 box, default value after installation is 1048576. When we hit this fallback code, it takes almost a full second to spawn a single child process. In fork intensive scenarios this can get pretty bad. What do you think? Thanks & Best Regards, Thomas

4 11

[12] RFR of JDK-8210353: Move java/util/Arrays/TimSortStackSize2.java back to tier1
by Amy Lu 23 Oct '18

23 Oct '18

test/jdk/java/util/Arrays/TimSortStackSize2.java This test was demoted to tier2 due to JDK-8199265 (test fails with OOM). This issue has been fixed (in jdk-11+20) and test has been run (in tier2) without failure happening after that. Let's move it back to tier1. bug: https://bugs.openjdk.java.net/browse/JDK-8210353 webrev: http://cr.openjdk.java.net/~amlu/8210353/webrev.00/ Thanks, Amy diff --git a/test/jdk/TEST.groups b/test/jdk/TEST.groups --- a/test/jdk/TEST.groups +++ b/test/jdk/TEST.groups @@ -35,8 +35,7 @@ :jdk_lang tier1_part2 = \ - :jdk_util \ - -java/util/Arrays/TimSortStackSize2.java + :jdk_util tier1_part3 = \ :jdk_math \ @@ -67,9 +66,7 @@ -sun/nio/cs/ISO8859x.java \ :jdk_other \ :jdk_text \ - :jdk_time \ - java/util/Arrays/TimSortStackSize2.java - + :jdk_time tier2_part3 = \ :jdk_net diff --git a/test/jdk/java/util/Arrays/TimSortStackSize2.java b/test/jdk/java/util/Arrays/TimSortStackSize2.java --- a/test/jdk/java/util/Arrays/TimSortStackSize2.java +++ b/test/jdk/java/util/Arrays/TimSortStackSize2.java @@ -25,7 +25,6 @@ * @test * @bug 8072909 * @summary Test TimSort stack size on big arrays - * @key intermittent * @library /lib/testlibrary /test/lib * @modules java.management * java.base/jdk.internal

3 5

JDK-6202130: java.util.jar.Attributes.writeMain() can't handle multi-byte chars
by Philipp Kunz 22 Oct '18

22 Oct '18

Hi, I tried to fix bug 6202130 about manifest utf support and come up now with a test as suggested in the bug's comments that shows that utf charset actually works before removing the comments from the code. When I wanted to remove the XXX comments about utf it occurred to me that version attributes ("Signature-Version" and "Manifest-Version") would never be broken across lines and should anyway not support the whole utf character set which sounds more like related to bugs 6910466 or 4935610 but it's not a real fit. Therefore, I could not remove one such comment of Attributes#writeMain but I changed it. The first comment in bug 6202130 mentions only two comments but there are three in Attributes. In the attached patch I removed only two of three and changed the remaining third to not mention utf anymore. At the moment, at least until 6443578 is fixed, multi-byte utf characters can be broken across lines. It might be worth a consideration to test that explicitly as well but then I guess there is not much of a point in testing the current behavior that will change with 6443578, hopefully soon. There are in my opinion enough characters broken across lines in the attached test that demonstrate that this still works like it did before. I would have preferred also to remove the calls to deprecated String(byte[], int, int, int) but then figured it relates more to bug 6443578 than 6202130 and now prefer to do that in another separate patch. Bug 6202130 also states that lines are broken by String.length not by byte length. While it looks so at first glance, I could not confirm. The combination of getBytes("UTF8"), String(byte[], int, int, int), and then DataOutputStream.writeBytes(String) in that combination does not drop high-bytes because every byte (whether a whole character or only a part of a multi-byte character) becomes a character in String(...) containing that byte in its low-byte which will be read again from writeBytes(...). Or put in a different way, every utf encoded byte becomes a character and multi-byte utf characters are converted into multiple string characters containing one byte each in their lower bytes. The current solution is not nice, but at least works. With that respect I'd like to suggest to deprecate DataOutputStream.writeBytes(String) because it does something not exactly expected when guessing from its name and that would suit a byte[] parameter better very much like it has been done with String(byte[], int, int, int). Any advice about the procedure to deprecate something? I was surprised that it was not trivial to list all valid utf characters. If someone has a better idea than isValidUtfCharacter in the attached test, let me know. Altogether, I would not consider 6202130 resolved completely, unless maybe all remaining points are copied to 6443578 and maybe another bug about valid values for "Signature-Version" and "Manifest-Version" if at all desired. But still I consider the attached patch an improvement and most of the remainder can then be solved in 6443578 and so far I am looking forward to any kind of feedback. Regards, Philipp

5 19

[12] RFR 8208542: Add 4 JNDI tests to com/sun/jndi/dns/ListTests/
by Chris Yin 19 Oct '18

19 Oct '18

Please review the changes to add 4 JNDI tests to com/sun/jndi/dns/ListTests/ in OpenJDK, thanks bug: https://bugs.openjdk.java.net/browse/JDK-8208542 webrev: http://cr.openjdk.java.net/~xyin/8208542/webrev.00/ Regards, Chris

3 6

[12] RFR 8210339: Add 10 JNDI tests to com/sun/jndi/dns/FedTests/
by Chris Yin 19 Oct '18

19 Oct '18

Please review the changes to add 10 JNDI tests to com/sun/jndi/dns/FedTests/ in OpenJDK, thanks bug: https://bugs.openjdk.java.net/browse/JDK-8210339 webrev: http://cr.openjdk.java.net/~xyin/8210339/webrev.00/ Regards, Chris

3 4

[12] RFR 8200151: Add 8 JNDI tests to com/sun/jndi/dns/ConfigTests/
by Chris Yin 19 Oct '18

19 Oct '18

Please review the changes to add 8 JNDI tests to com/sun/jndi/dns/ConfigTests/ in OpenJDK, due to known issue 7164518, PortUnreachable.java will be problem list for 'macosx-all', thanks bug: https://bugs.openjdk.java.net/browse/JDK-8200151 <https://bugs.openjdk.java.net/browse/JDK-8200151> webrev: http://cr.openjdk.java.net/~xyin/8200151/webrev.00/ <http://cr.openjdk.java.net/~xyin/8200151/webrev.00/> Regards, Chris

3 9

RFR (Enhancement): 6194856: Zip Files lose ALL ownership and permissions of the files
by Langer, Christoph 12 Oct '18

12 Oct '18

Hi all, I had asked for opinions regarding adding posix permission support to JDK’s zip handling libraries and tools [1]. Since I didn’t get clear “no, you can’t do this” answers, I’m now concretely proposing some enhancements in the area of java.util.zip, jdk.zipfs and jdk.jartool. I have reopened the long standing bug report (6194856) which had been set to “Won’t fix” quite recently for this purpose. Here are the technical details: The ZIP format specifications by infozip and pkware ([2] and [3]) do not explicitly specify the handling of posix file permissions. But it seems to be common sense that if file attribute compatibility is set to “Unix” in the upper byte of CEN field “version made by”, the file permissions bits are stored in CEN field “external file attributes”, byte 3 and 4. My changes try to honor this in the least obtrusive way. 😊 The following changes are proposed: java.util.zip.ZipEntry: it will have an additional field “posixPerms”. A value of -1 means “no permission information”, positive values will contain the flag values. There will be 2 new public methods to read/set the permission information: public Optional<Set<PosixFilePermission>> getPosixPermissions() public void setPosixPermissions(Set<PosixFilePermission> permissions) The usage of type “Optional” reflects that posix permissions are not necessarily present in a zip file java.util.zip.ZipFile: it will have the capability to read the CEN fields and set posixPerms if available java.util.zip.ZipOutputStream: it will store entries with associated posix permissions as unix type in the CEN, together with the bit mask for the permissions jdk.jartool: I propose to add and option "--preserve-posix" or short "-o" to honor the posix bits that may be stored inside zip/jar files. By default the option is not set and hence posix permissions are ignored. If the flag is set and the file system that the jar tool is running on supports posix, posix file permissions that exist in the file system will be stored in newly created/update archives or restored to the file system if such information is present in the archive. jdk.zipfs: I added the capability for posix file permissions in the implementation. I decided to support PosixFileAttributes by subclassing ZipFileAttributes from this superclass as well as subclassing ZipFileAttributeView from PosixFileAttributeView. However, as PosixFileAttributes also include groups and owners, I would throw UnsupportedOperationExceptions in case of invoking methods to handle these attributes. But this approach seems to be most consistent with e.g. Files.setPosixFilePermissions and Files.getPosixFilePermissions. java.nio.file.attribute.PosixFilePermissions: As this class presents a collection of static helpers, I added definitions for the posix file bit masks together with methods to convert between Sets of PosixFilePermission to bit masks containing the according switches and vice versa. These definitions could theoretically also be moved inside the java.util.zip or jdk.zipfs implementations where they wouldn’t be exposed as public APIs. However, in that case the code would probably need to be duplicated. I’ve also created two jtreg testcases for both, java.util.zip and jdk.nio.zipfs. The changes also contain a few further code cleanups. Here are the links: Bug: https://bugs.openjdk.java.net/browse/JDK-6194856 Webrev: http://cr.openjdk.java.net/~clanger/webrevs/6194856.0/ I’ll write a CSR once there’s some substantial feedback to my endeavor. Thanks in advance for reviewing/commenting. Best regards Christoph [1] http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-September/055375.… [2] http://www.info-zip.org/doc/appnote-19970311-iz.zip [3] https://pkware.cachefly.net/webdocs/casestudies/APPNOTE.TXT

3 7

RFR(M) : 8211171 : move JarUtils to top-level testlibrary
by Igor Ignatyev 11 Oct '18

11 Oct '18

http://cr.openjdk.java.net/~iignatyev//8211171/webrev.00/index.html > 182 lines changed: 0 ins; 152 del; 30 mod; Hi all, could you please review this small and straightforward patch which moves JarUtils from jdk testlibrary to the top-level testlibrary, removes JarUtils from jaxp testlibrary, and updates all the affected tests? please note, there is another version of JarUtils in jdk.test.lib.util package, but these two versions have different signatures and behavior, merging them is a bit tricker and will be done separately. JBS: https://bugs.openjdk.java.net/browse/JDK-8211171 webrev: http://cr.openjdk.java.net/~iignatyev//8211171/webrev.00/index.html testing: tier1-3 Thanks, -- Igor

4 9