Re: RFR: 8270490: Charset.forName() taking fallback default value [v2]
On Wed, 20 Oct 2021 19:02:30 GMT, Naoto Sato wrote:
>> During the review of JEP 400, a proposal to provide an overloaded method to
>> `Charset.forName()` was suggested
>> [[1]](https://github.com/openjdk/jdk/pull/4733#discussion_r669693954). This
>> PR is to implement the proposal. A CSR is also drafted as
>> https://bugs.openjdk.java.net/browse/JDK-8275348
>
> Naoto Sato has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Moved the null sentence into @param tag.
Oh, I found that I checked the outdated source code. Now this problem does not
exist in `StringCoding`.
I simply browsed the latest source code of JDK and found that this idiom no
longer appears outside jline. I believe that the source code of jline does not
need to be modified in openjdk.
But I noticed `LauncherHelper.makePlatformString`
(https://github.com/openjdk/jdk/blob/c978ca87de2d9152345dfd85983278c42bb28cd3/src/java.base/share/classes/sun/launcher/LauncherHelper.java#L887)
I don't understand why it stores `encoding` string and `isCharsetSupported`
boolean values. Nor do I find references to these two fields in native code. I
think it can benefit from the improvement in this PR like this?
private static final String encprop = "sun.jnu.encoding";
private static Charset charset = null;
/*
* converts a c or a byte array to a platform specific string,
* previously implemented as a native method in the launcher.
*/
static String makePlatformString(boolean printToStderr, byte[] inArray) {
initOutput(printToStderr);
if (charset == null) {
charset = Charset.forName(encprop, Charset.defaultCharset());
}
return new String(inArray, charset);
}
-
PR: https://git.openjdk.java.net/jdk/pull/6045
Re: JDK-8275509: (jlink) SystemModulesPlugin generates a jdk.internal.module.SystemModules$all.class which isn't reproducible
Hello Ioi,
On 21/10/21 9:59 pm, Ioi Lam wrote:
On 10/21/21 5:25 AM, Alan Bateman wrote:
On 21/10/2021 10:49, Jaikiran Pai wrote:
:
Digging into it, it appears that since the ModuleDescriptor#equals()
calls equals() on enum types (in this specific case on
ModuleDescriptor.Requires.Modifier) and since enum type equality is
implemented as identity checks, those identity checks are
surprisingly failing. More specifically
ModuleDescriptor.Requires.Modifier.MANDATED ==
ModuleDescriptor.Requires.Modifier.MANDATED is equating to false
because at runtime I see that two different instances of
ModuleDescriptor.Requires.Modifier.MANDATED have been loaded (by the
same boot module classloader). Although I use
ModuleDescriptor.Requires.Modifier.MANDATED as an example, the same
is reproducible with other enum values like
ModuleDescriptor.Requires.Modifier.TRANSITIVE.
This appears to be specific to CDS since running the above program
with:
java -Xshare:off EnumEquality
succeeds and the ModuleDescriptor equality check passes.
In short, it looks like there is some general issue with CDS and
equality checks with enums and perhaps deserves a separate JBS issue?
I've asked Ioi Lam to comment on this, off-hand I'm not aware of any
issues with CDS here but it may be related to the archiving of object
graphs.
-Alan
Hi Jaikiran and Alan,
Thanks for reporting this issue. It's a bug in CDS. I have filed
https://bugs.openjdk.java.net/browse/JDK-8275731 and am working on a fix.
Thank you for looking into this.
This is my initial analysis of the problem.
>>> Can anyone think of similar problems that may happen elsewhere?
The static constructors of enum classes are executed at both CDS dump
time and run time. E.g.,
public enum Modifier {
OPEN
}
The method essentially does this:
public static final Modifier OPEN = new Modifier("OPEN");
If a reference of Modifier.OPEN is stored inside the CDS archived heap
during dump time, it will be different than the value of Modifier.OPEN
that is re-created at runtime by the execution of Modifier.
I have almost next to nothing knowledge about CDS internals. My only
understanding of it is based on some documentation that I have read. One
of them being this one
https://docs.oracle.com/en/java/javase/17/vm/class-data-sharing.html#GUID-7EAA3411-8CF0-4D19-BD05-DF5E1780AA91.
Based on that documentation (and other similar ones), it was my
understanding that CDS was meant to store/share class "metadata" like it
states in that doc:
"When the JVM starts, the shared archive is memory-mapped to allow
sharing of read-only JVM metadata for these classes among multiple JVM
processes."
But from what you explain in that enum example, it looks like it also
stores class instance data that is computed at build time on the host
where the JDK image was generated? Did I understand it correctly? Is
this only for enums or does it also store the static initialization data
of "class" types too? If it does store the static init data of class
types too, then wouldn't such data be host/build time specific and as
such the classes that need to be enrolled into the default CDS archive
of the JDK should be very selective (by reviewing what they do in their
static init)? Like I said, I haven't looked into this in detail so
perhaps it already is selective in the JDK build?
-Jaikiran
Re: RFR: 8270490: Charset.forName() taking fallback default value [v2]
On Wed, 20 Oct 2021 19:02:30 GMT, Naoto Sato wrote:
>> During the review of JEP 400, a proposal to provide an overloaded method to
>> `Charset.forName()` was suggested
>> [[1]](https://github.com/openjdk/jdk/pull/4733#discussion_r669693954). This
>> PR is to implement the proposal. A CSR is also drafted as
>> https://bugs.openjdk.java.net/browse/JDK-8275348
>
> Naoto Sato has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Moved the null sentence into @param tag.
I'm not reviewer.
I think maybe we should check all the usage of `Charset.isSupported`.
At present, `Charset.isSupported` and `Charset.forName` are often used
continuously, such as in `StringCoding`:
class StringCoding {
// ...
private static Charset lookupCharset(String csn) {
if (Charset.isSupported(csn)) {
try {
return Charset.forName(csn);
} catch (UnsupportedCharsetException x) {
throw new Error(x);
}
}
return null;
}
//...
}
This calls `Charset.lookup` twice. Replacing it with such code should eliminate
unnecessary lookup:
private static Charset lookupCharset(String csn) {
return Charset.forName(csn, null);
}
-
PR: https://git.openjdk.java.net/jdk/pull/6045
Make j.l.i.CallSite class sealed
Currently, the java.lang.invoke.CallSite has a package-private constructor, and in its Javadoc it already mentions all the three subclasses: ConstantCallSite, MutableCallSite and VolatileCallSite. I guess that now that Java has support for sealed classes, the CallSite could be a candidate for it too. So CallSite would be sealed and the permitted subclasses would be non-sealed, to allow existing behavior. https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/lang/invoke/CallSite.html
Re: RFR: 8270490: Charset.forName() taking fallback default value [v2]
On Thu, 21 Oct 2021 16:03:29 GMT, Naoto Sato wrote:
>> Apparently `IllegalCharsetNameException` or `IllegalArgumentException` could
>> still be thrown - so removing the `try-catch` would be a change of behaviour
>> in those cases. It all depends on whether there is a chance that these
>> exceptions could be thrown in this particular context (with these particular
>> input parameters) - which I am not able to tell - but maybe someone more
>> familiar with this code could...
>
> I first thought of swallowing all exceptions in 2-arg forName(), but decided
> not to do that. Because `IllegalArgumentException` and
> `IllegalCharsetNameException` are for the validity of the passed
> `charsetName`, like detecting `null` or invalid chars like "😱". On the other
> hand, `UnsupportedCharsetException` is for the availability which varies
> depending on the user's settings and or platform, which can be safely
> replaced with `fallback` charset. So yes, it is not totally getting rid of
> `try-catch` but it avoids `UnsupportedCharsetException` which is only
> detectable at runtime.
Then what is the benefit, if the user will have to write such code anyway?:
try {
cs = Charset.forName(StaticProperty.nativeEncoding(), fallback);
} catch (Exception ignored) {
cs = fallback;
}
Even in the current code update it can work well w/o the second parameter.
-
PR: https://git.openjdk.java.net/jdk/pull/6045
Re: RFR: 8275650: Problemlist java/io/File/createTempFile/SpecialTempFile.java for Windows 11
On Wed, 20 Oct 2021 00:04:08 GMT, Ivan Šipka wrote: > cc @ctornqvi as it has been discussed internally, jtreg doesn’t recognize $os-$arch-$version pattern in problem list (but understands $os-$version) so for the test to be excluded only on windows 11, you should use `windows-11` - Changes requested by iignatyev (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/6025
Re: RFR: 8273111: Default timezone should return zone ID if /etc/localtime is valid but not canonicalization on linux
On Wed, 1 Sep 2021 13:39:46 GMT, Naoto Sato wrote: >> Hi, >> Please help me review the change to enhance getting time zone ID from >> /etc/localtime on linux. >> >> We use `realpath` instead of `readlink` to obtain the link name of >> /etc/localtime, because `readlink` can only read the value of a symbolic of >> link, not the canonicalized absolute pathname. >> >> For example, the value of /etc/localtime is >> "../usr/share/zoneinfo//Asia/Shanghai", then the linkbuf obtained by >> `readlink` is "../usr/share/zoneinfo//Asia/Shanghai", and then the call of >> `getZoneName(linkbuf)` will get "/Asia/Shanghai", not "Asia/Shanghai", which >> consider as invalid in `ZoneInfoFile.getZoneInfo()`. Using `realpath`, you >> can get “/usr/share/zoneinfo/Asia/Shanghai“ directly from “/etc/localtime“. >> >> Thanks, >> wuyan > > The change looks reasonable. Please test your fix with macOS as well. Hi, @naotoj, I pushed the new commit, do you need to review again? - PR: https://git.openjdk.java.net/jdk/pull/5327
Re: RFR: 8270490: Charset.forName() taking fallback default value [v2]
On Thu, 21 Oct 2021 18:00:46 GMT, Naoto Sato wrote:
>> I'm not reviewer.
>>
>> I'd like to write down following code without `try-catch`.
>>
>> var cs = Charset.forName(charsetName, null);
>> if (cs == null) cs = StandardCharsets.UTF_8;
>>
>> or please find out more easy way.
>
>> I'd like to write down following code without `try-catch`.
>
> You don't *have to* try-catch those exceptions if you are not interested, as
> they are subclasses of `RuntimeException`.
>
>> ```
>> var cs = Charset.forName(charsetName, null);
>> if (cs == null) cs = StandardCharsets.UTF_8;
>> ```
>
> This could be simplified to
>
>
> var cs = Charset.forName(charsetName, StandardCharsets.UTF_8);
@naotoj
Oh sorry, I'd like to detect fallback charset is used, like:
var cs = Charset.forName(charsetName, null);
if (cs == null) {
System.err.println("Used UTF-8 encoding instead of "+charsetName+");
cs = StandardCharsets.UTF_8;
}
-
PR: https://git.openjdk.java.net/jdk/pull/6045
Re: RFR: 4511638: Double.toString(double) sometimes produces incorrect results [v3]
> Hello, > > here's a PR for a patch submitted on March 2020 > [1](https://cr.openjdk.java.net/~bpb/4511638/webrev.04/) when Mercurial was a > thing. > > The patch has been edited to adhere to OpenJDK code conventions about > multi-line (block) comments. Nothing in the code proper has changed, except > for the addition of redundant but clarifying parentheses in some expressions. > > > Greetings > Raffaello Raffaello Giulietti has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains seven commits: - 4511638: Double.toString(double) sometimes produces incorrect results Adjusted hashes in test/langtools/tools/javac/sym/ElementStructureTest.java - 4511638: Double.toString(double) sometimes produces incorrect results Merge branch 'master' into JDK-4511638 - 4511638: Double.toString(double) sometimes produces incorrect results - 4511638: Double.toString(double) sometimes produces incorrect results Refactored test classes to better match OpenJDK conventions. Added tests recommended by Guy Steele and Paul Zimmermann. - Changed MAX_CHARS to static - 4511638: Double.toString(double) sometimes produces incorrect results - 4511638: Double.toString(double) sometimes produces incorrect results - Changes: https://git.openjdk.java.net/jdk/pull/3402/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=3402&range=02 Stats: 23941 lines in 16 files changed: 23805 ins; 61 del; 75 mod Patch: https://git.openjdk.java.net/jdk/pull/3402.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3402/head:pull/3402 PR: https://git.openjdk.java.net/jdk/pull/3402
Re: RFR: JDK-8263155: Allow additional contents for DMG [v2]
On Thu, 21 Oct 2021 15:36:36 GMT, Andy Herrick wrote: >> JDK-8263155: Allow additional contents for DMG > > Andy Herrick has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains five commits: > > - Merge branch 'master' into JDK-8263155 > - Merge branch 'master' into JDK-8263155 > - JDK-8263155: Allow additional contents for DMG > - JDK-8263155: Allow additional contents for DMG > - JDK-8263155: Allow additional contents for DMG Marked as reviewed by almatvee (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/5912
Re: RFR: 8244202: Implementation of JEP 418: Internet-Address Resolution SPI [v6]
On Thu, 21 Oct 2021 18:21:50 GMT, Aleksei Efimov wrote:
>> This change implements a new service provider interface for host name and
>> address resolution, so that java.net.InetAddress API can make use of
>> resolvers other than the platform's built-in resolver.
>>
>> The following API classes are added to `java.net.spi` package to facilitate
>> this:
>> - `InetAddressResolverProvider` - abstract class defining a service, and
>> is, essentially, a factory for `InetAddressResolver` resolvers.
>> - `InetAddressResolverProvider.Configuration ` - an interface describing the
>> platform's built-in configuration for resolution operations that could be
>> used to bootstrap a resolver construction, or to implement partial
>> delegation of lookup operations.
>> - `InetAddressResolver` - an interface that defines methods for the
>> fundamental forward and reverse lookup operations.
>> - `InetAddressResolver.LookupPolicy` - a class whose instances describe the
>> characteristics of one forward lookup operation.
>>
>> More details in [JEP-418](https://openjdk.java.net/jeps/418).
>>
>> Testing: new and existing `tier1:tier3` tests
>
> Aleksei Efimov has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Review updates + move resolver docs to the provider class (CSR update to
> follow)
src/java.base/share/classes/java/net/InetAddress.java line 152:
> 150: *
> 151: * Host Name Resolution
> 152: * Host name-to-IP address resolution is accomplished through the
> use
maybe you need a ` ` there even though it's the first paragaph under the
header.
src/java.base/share/classes/java/net/InetAddress.java line 159:
> 157: * by href="spi/InetAddressResolverProvider.html#system-wide-resolver">
> 158: * deploying an {@link InetAddressResolverProvider}.
> 159: * The built-in resolver implementation is
> used by
did you double check that anchors defined with `` actually work in
the generated API doc?
Also maybe leave a blank line before any opening `` tag...
src/java.base/share/classes/java/net/spi/InetAddressResolver.java line 36:
> 34: * This interface defines operations for looking-up host names and IP
> addresses.
> 35: * An instance of {@code InetAddressResolver} is
> 36: * href="InetAddressResolverProvider.html#system-wide-resolver">installed as
> a
Maybe it would be more appropriate to link _system-wide-resolver_ instead (on
the next line in the same sentence).
src/java.base/share/classes/java/net/spi/InetAddressResolverProvider.java line
37:
> 35: *
> 36: * A resolver provider is a factory for custom implementations of
> {@linkplain
> 37: * InetAddressResolver resolvers}. A resolver define operations for
> looking up
... a resolver define**s** ...
-
PR: https://git.openjdk.java.net/jdk/pull/5822
Re: RFR: JDK-8263155: Allow additional contents for DMG [v2]
On Thu, 21 Oct 2021 15:36:36 GMT, Andy Herrick wrote: >> JDK-8263155: Allow additional contents for DMG > > Andy Herrick has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains five commits: > > - Merge branch 'master' into JDK-8263155 > - Merge branch 'master' into JDK-8263155 > - JDK-8263155: Allow additional contents for DMG > - JDK-8263155: Allow additional contents for DMG > - JDK-8263155: Allow additional contents for DMG Marked as reviewed by asemenyuk (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/5912
Re: RFR: 8244202: Implementation of JEP 418: Internet-Address Resolution SPI [v6]
> This change implements a new service provider interface for host name and > address resolution, so that java.net.InetAddress API can make use of > resolvers other than the platform's built-in resolver. > > The following API classes are added to `java.net.spi` package to facilitate > this: > - `InetAddressResolverProvider` - abstract class defining a service, and is, > essentially, a factory for `InetAddressResolver` resolvers. > - `InetAddressResolverProvider.Configuration ` - an interface describing the > platform's built-in configuration for resolution operations that could be > used to bootstrap a resolver construction, or to implement partial delegation > of lookup operations. > - `InetAddressResolver` - an interface that defines methods for the > fundamental forward and reverse lookup operations. > - `InetAddressResolver.LookupPolicy` - a class whose instances describe the > characteristics of one forward lookup operation. > > More details in [JEP-418](https://openjdk.java.net/jeps/418). > > Testing: new and existing `tier1:tier3` tests Aleksei Efimov has updated the pull request incrementally with one additional commit since the last revision: Review updates + move resolver docs to the provider class (CSR update to follow) - Changes: - all: https://git.openjdk.java.net/jdk/pull/5822/files - new: https://git.openjdk.java.net/jdk/pull/5822/files/30226481..2a554c91 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=5822&range=05 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5822&range=04-05 Stats: 123 lines in 3 files changed: 49 ins; 40 del; 34 mod Patch: https://git.openjdk.java.net/jdk/pull/5822.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5822/head:pull/5822 PR: https://git.openjdk.java.net/jdk/pull/5822
Re: RFR: 8244202: Implementation of JEP 418: Internet-Address Resolution SPI [v5]
On Wed, 20 Oct 2021 18:47:32 GMT, Alan Bateman wrote: >> Aleksei Efimov has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Change InetAddressResolver method names > > src/java.base/share/classes/java/net/InetAddress.java line 244: > >> 242: * @implNote >> 243: * For any lookup operation that might occur before the VM is fully >> booted the built-in >> 244: * resolver will be used. > > I think we will need decide if InetAddress class description is the right > place for this or whether some/most of it should move to > InetAddressResolverProvider. It might be that we update the existing "Host > Name Resolution" section with some basic/readable text to make the reader > aware that there is a provider mechanism with a link to > InetAddressResolverProvider with the details. Thanks for a good suggestion, Alan. It looks better now (2a554c91864e3b42a0ea315b0a671782fe341927) with some basic text for provider mechanism added to `Host Name Resolution`, and `InetAddress Resolver Providers` section moved to `InetAddressResolverProvider`. - PR: https://git.openjdk.java.net/jdk/pull/5822
Re: RFR: 8270490: Charset.forName() taking fallback default value [v2]
On Thu, 21 Oct 2021 16:06:31 GMT, Ichiroh Takiguchi wrote: > I'd like to write down following code without `try-catch`. You don't *have to* try-catch those exceptions if you are not interested, as they are subclasses of `RuntimeException`. > ``` > var cs = Charset.forName(charsetName, null); > if (cs == null) cs = StandardCharsets.UTF_8; > ``` This could be simplified to var cs = Charset.forName(charsetName, StandardCharsets.UTF_8); - PR: https://git.openjdk.java.net/jdk/pull/6045
Re: JDK-8275509: (jlink) SystemModulesPlugin generates a jdk.internal.module.SystemModules$all.class which isn't reproducible
On 10/21/21 5:25 AM, Alan Bateman wrote:
On 21/10/2021 10:49, Jaikiran Pai wrote:
:
Digging into it, it appears that since the ModuleDescriptor#equals()
calls equals() on enum types (in this specific case on
ModuleDescriptor.Requires.Modifier) and since enum type equality is
implemented as identity checks, those identity checks are
surprisingly failing. More specifically
ModuleDescriptor.Requires.Modifier.MANDATED ==
ModuleDescriptor.Requires.Modifier.MANDATED is equating to false
because at runtime I see that two different instances of
ModuleDescriptor.Requires.Modifier.MANDATED have been loaded (by the
same boot module classloader). Although I use
ModuleDescriptor.Requires.Modifier.MANDATED as an example, the same
is reproducible with other enum values like
ModuleDescriptor.Requires.Modifier.TRANSITIVE.
This appears to be specific to CDS since running the above program with:
java -Xshare:off EnumEquality
succeeds and the ModuleDescriptor equality check passes.
In short, it looks like there is some general issue with CDS and
equality checks with enums and perhaps deserves a separate JBS issue?
I've asked Ioi Lam to comment on this, off-hand I'm not aware of any
issues with CDS here but it may be related to the archiving of object
graphs.
-Alan
Hi Jaikiran and Alan,
Thanks for reporting this issue. It's a bug in CDS. I have filed
https://bugs.openjdk.java.net/browse/JDK-8275731 and am working on a fix.
This is my initial analysis of the problem.
>>> Can anyone think of similar problems that may happen elsewhere?
The static constructors of enum classes are executed at both CDS dump
time and run time. E.g.,
public enum Modifier {
OPEN
}
The method essentially does this:
public static final Modifier OPEN = new Modifier("OPEN");
If a reference of Modifier.OPEN is stored inside the CDS archived heap
during dump time, it will be different than the value of Modifier.OPEN
that is re-created at runtime by the execution of Modifier.
Thanks
- Ioi
Re: RFR: 8274544: Langtools command's usage were garbled on Japanese Windows [v3]
On Tue, 19 Oct 2021 01:26:35 GMT, Jonathan Gibbons wrote:
>> Ichiroh Takiguchi has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> 8274544: Langtools command's usage were garbled on Japanese Windows
>
> This is pretty ugly code to be replicating so many times.
>
> What if the tools have been run in an environment where `System.out` and
> `System.err` have already been redirected in some manner, with
> `System.setOut` or `System.setErr`? You should not assume that `System.out`
> and `System.err` will always refer to the console.
@jonathan-gibbons I appreciate your comment.
I'd like to confirm something.
> This is pretty ugly code to be replicating so many times.
> What if the tools have been run in an environment where `System.out` and
> `System.err` have already been redirected in some manner, with
> `System.setOut` or `System.setErr`? You should not assume that `System.out`
> and `System.err` will always refer to the console.
I was confused since the fixed code did not call System.out/System.err directly.
I tried following code on Japanese Windows.
import java.io.*;
import java.nio.charset.*;
public class OutputCheck {
public static void main(String[] args) throws Exception {
String s = "\u3042";
System.out.println("[1]:"+s);
PrintStream ps = System.out;
System.setOut(new PrintStream(System.out));
System.out.println("[2]:"+s);
ps.println("[3]:"+s);
System.setOut(new PrintStream(System.out, true,
Charset.forName(System.getProperty("native.encoding";
System.out.println("[4]:"+s);
}
}
Output is:
> jdk-18-b14\bin\java OutputCheck.java
[1]:あ
[2]:縺・
[3]:あ
[4]:あ
[2] refers default charset (UTF-8)
[3] is same as [1]
[4] specifies native.encoding system encoding
Could you explain more detail ?
-
PR: https://git.openjdk.java.net/jdk/pull/5771
Re: RFR: 8270490: Charset.forName() taking fallback default value [v2]
On Wed, 20 Oct 2021 19:02:30 GMT, Naoto Sato wrote: >> During the review of JEP 400, a proposal to provide an overloaded method to >> `Charset.forName()` was suggested >> [[1]](https://github.com/openjdk/jdk/pull/4733#discussion_r669693954). This >> PR is to implement the proposal. A CSR is also drafted as >> https://bugs.openjdk.java.net/browse/JDK-8275348 > > Naoto Sato has updated the pull request incrementally with one additional > commit since the last revision: > > Moved the null sentence into @param tag. I'm not reviewer. I'd like to write down following code without `try-catch`. var cs = Charset.forName(charsetName, null); if (cs == null) cs = StandardCharsets.UTF_8; or please find out more easy way. - PR: https://git.openjdk.java.net/jdk/pull/6045
Re: RFR: 8270490: Charset.forName() taking fallback default value [v2]
On Thu, 21 Oct 2021 09:33:30 GMT, Daniel Fuchs wrote: >> Right, I think both try-catch usages will be removed. > > Apparently `IllegalCharsetNameException` or `IllegalArgumentException` could > still be thrown - so removing the `try-catch` would be a change of behaviour > in those cases. It all depends on whether there is a chance that these > exceptions could be thrown in this particular context (with these particular > input parameters) - which I am not able to tell - but maybe someone more > familiar with this code could... I first thought of swallowing all exceptions in 2-arg forName(), but decided not to do that. Because `IllegalArgumentException` and `IllegalCharsetNameException` are for the validity of the passed `charsetName`, like detecting `null` or invalid chars like "😱". On the other hand, `UnsupportedCharsetException` is for the availability which varies depending on the user's settings and or platform, which can be safely replaced with `fallback` charset. So yes, it is not totally getting rid of `try-catch` but it avoids `UnsupportedCharsetException` which is only detectable at runtime. - PR: https://git.openjdk.java.net/jdk/pull/6045
Re: RFR: 8270490: Charset.forName() taking fallback default value [v2]
On Thu, 21 Oct 2021 15:00:03 GMT, Roger Riggs wrote:
>> src/java.base/share/classes/java/io/Console.java line 594:
>>
>>> 592: cs = Charset.forName(StaticProperty.nativeEncoding(),
>>> Charset.defaultCharset());
>>> 593: } catch (Exception ignored) {
>>> 594: cs = Charset.defaultCharset();
>>
>> What kind of actual improvements do we get here since the catch block is
>> still in place?
>
> In the case of Console, both charset names come from system properties and
> could refer to invalid or unavailable charsets. (null is handled
> separately). The code silently ignores the invalid values. The new method ,
> as is, is not a fully satisfying replacement. Catching Exception is too
> broad a catch but may be warranted in this case so that some Console charset
> is selected.
>
> The new method would be useful in more cases if the default was returned for
> any of
> IllegalCharsetNameException, IllegalArgumentException, and
> UnsupportedCharsetException.
Since we do support all the encodings for platforms we support out-of-the-box,
it could still be possible that the user can specify his/her console encoding
to the one we do not support. In that case, we can safely use the default
`UTF-8` without throwing/catching `UnsupportedCharsetException`.
-
PR: https://git.openjdk.java.net/jdk/pull/6045
Re: RFR: JDK-8263155: Allow additional contents for DMG [v2]
> JDK-8263155: Allow additional contents for DMG Andy Herrick has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains five commits: - Merge branch 'master' into JDK-8263155 - Merge branch 'master' into JDK-8263155 - JDK-8263155: Allow additional contents for DMG - JDK-8263155: Allow additional contents for DMG - JDK-8263155: Allow additional contents for DMG - Changes: https://git.openjdk.java.net/jdk/pull/5912/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=5912&range=01 Stats: 171 lines in 8 files changed: 154 ins; 1 del; 16 mod Patch: https://git.openjdk.java.net/jdk/pull/5912.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5912/head:pull/5912 PR: https://git.openjdk.java.net/jdk/pull/5912
Integrated: JDK-8275688: Suppress warnings on non-serializable non-transient instance fields in DualPivotQuicksort
On Thu, 21 Oct 2021 04:40:25 GMT, Joe Darcy wrote: > This should be the last core libraries cleanup of non-serializable > non-transient instance fields ahead of the upcoming javac lint warnings: > https://mail.openjdk.java.net/pipermail/jdk-dev/2021-October/006166.html This pull request has now been integrated. Changeset: 0761a4b9 Author:Joe Darcy URL: https://git.openjdk.java.net/jdk/commit/0761a4b915217abb08ef9b5c1a60878aedf5572c Stats: 4 lines in 1 file changed: 3 ins; 0 del; 1 mod 8275688: Suppress warnings on non-serializable non-transient instance fields in DualPivotQuicksort Reviewed-by: rriggs - PR: https://git.openjdk.java.net/jdk/pull/6058
Integrated: JDK-8275686: Suppress warnings on non-serializable non-transient instance fields in java.rmi
On Thu, 21 Oct 2021 01:06:33 GMT, Joe Darcy wrote: > Another serialization warnings cleanup; this time in java.rmi. This pull request has now been integrated. Changeset: 3cb241a9 Author:Joe Darcy URL: https://git.openjdk.java.net/jdk/commit/3cb241a91fd2cc6b0b3b333288028694e60f723f Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod 8275686: Suppress warnings on non-serializable non-transient instance fields in java.rmi Reviewed-by: bpb, iris, rriggs - PR: https://git.openjdk.java.net/jdk/pull/6055
Re: RFR: 8275063: Implementation of Foreign Function & Memory API (Second incubator) [v9]
> This PR contains the API and implementation changes for JEP-419 [1]. A more > detailed description of such changes, to avoid repetitions during the review > process, is included as a separate comment. > > [1] - https://openjdk.java.net/jeps/419 Maurizio Cimadamore 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 15 additional commits since the last revision: - Fix regression in VaList treatment on AArch64 (contributed by @nick-arm) - Merge branch 'master' into JEP-419 - Fix copyright header in TestArrayCopy - Fix failing microbenchmarks. Contributed by @FrauBoes (thanks!) - * use `invokeWithArguments` to simplify new test - Add test for liveness check with high-aririty downcalls (make sure that if an exception occurs in a downcall because of liveness, ref count of other resources are left intact). - * Fix javadoc issue in VaList * Fix bug in concurrent logic for shared scope acquire - Address review comments - Apply suggestions from code review Co-authored-by: Paul Sandoz - Fix TestLinkToNativeRBP test - ... and 5 more: https://git.openjdk.java.net/jdk/compare/aeabb3df...5c69eabf - Changes: - all: https://git.openjdk.java.net/jdk/pull/5907/files - new: https://git.openjdk.java.net/jdk/pull/5907/files/414952ad..5c69eabf Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=5907&range=08 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5907&range=07-08 Stats: 25202 lines in 587 files changed: 18962 ins; 4240 del; 2000 mod Patch: https://git.openjdk.java.net/jdk/pull/5907.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5907/head:pull/5907 PR: https://git.openjdk.java.net/jdk/pull/5907
Re: RFR: JDK-8275688: Suppress warnings on non-serializable non-transient instance fields in DualPivotQuicksort [v2]
> This should be the last core libraries cleanup of non-serializable > non-transient instance fields ahead of the upcoming javac lint warnings: > https://mail.openjdk.java.net/pipermail/jdk-dev/2021-October/006166.html Joe Darcy has updated the pull request incrementally with one additional commit since the last revision: Update copyright. - Changes: - all: https://git.openjdk.java.net/jdk/pull/6058/files - new: https://git.openjdk.java.net/jdk/pull/6058/files/126782c3..72176dfe Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=6058&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=6058&range=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/6058.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6058/head:pull/6058 PR: https://git.openjdk.java.net/jdk/pull/6058
Re: RFR: 8270490: Charset.forName() taking fallback default value [v2]
On Thu, 21 Oct 2021 01:31:31 GMT, Sergey Bylokhov wrote:
>> Naoto Sato has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Moved the null sentence into @param tag.
>
> src/java.base/share/classes/java/io/Console.java line 594:
>
>> 592: cs = Charset.forName(StaticProperty.nativeEncoding(),
>> Charset.defaultCharset());
>> 593: } catch (Exception ignored) {
>> 594: cs = Charset.defaultCharset();
>
> What kind of actual improvements do we get here since the catch block is
> still in place?
In the case of Console, both charset names come from system properties and
could refer to invalid or unavailable charsets. (null is handled separately).
The code silently ignores the invalid values. The new method , as is, is not a
fully satisfying replacement. Catching Exception is too broad a catch but may
be warranted in this case so that some Console charset is selected.
The new method would be useful in more cases if the default was returned for
any of
IllegalCharsetNameException, IllegalArgumentException, and
UnsupportedCharsetException.
-
PR: https://git.openjdk.java.net/jdk/pull/6045
Integrated: 8270380: Change the default value of the java.security.manager system property to disallow
On Fri, 20 Aug 2021 22:44:34 GMT, Weijun Wang wrote: > This change modifies the default value of the `java.security.manager` system > property from "allow" to "disallow". This means unless it's explicitly set to > "allow", any call to `System.setSecurityManager()` would throw an UOE. > > This change was previously announced on the [jdk-dev > alias](https://mail.openjdk.java.net/pipermail/jdk-dev/2021-May/005616.html) > and is documented in [JEP 411](https://openjdk.java.net/jeps/411#Description). > > The `AllowSecurityManager.java` and `SecurityManagerWarnings.java` tests are > updated to confirm this behavior change. Two other tests are updated because > they were added after JDK-8267184 and do not have > `-Djava.security.manager=allow` on its `@run` line even it they need to > install a `SecurityManager` at runtime. This pull request has now been integrated. Changeset: d589b664 Author:Weijun Wang URL: https://git.openjdk.java.net/jdk/commit/d589b664cc809aea39ec094e99b1898df1bf3c19 Stats: 30 lines in 6 files changed: 5 ins; 8 del; 17 mod 8270380: Change the default value of the java.security.manager system property to disallow Reviewed-by: lancea, mullan, rriggs - PR: https://git.openjdk.java.net/jdk/pull/5204
Re: RFR: JDK-8275688: Suppress warnings on non-serializable non-transient instance fields in DualPivotQuicksort
On Thu, 21 Oct 2021 04:40:25 GMT, Joe Darcy wrote: > This should be the last core libraries cleanup of non-serializable > non-transient instance fields ahead of the upcoming javac lint warnings: > https://mail.openjdk.java.net/pipermail/jdk-dev/2021-October/006166.html Marked as reviewed by rriggs (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/6058
Re: RFR: JDK-8275686: Suppress warnings on non-serializable non-transient instance fields in java.rmi
On Thu, 21 Oct 2021 01:06:33 GMT, Joe Darcy wrote: > Another serialization warnings cleanup; this time in java.rmi. Marked as reviewed by rriggs (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/6055
Re: JDK-8275509: (jlink) SystemModulesPlugin generates a jdk.internal.module.SystemModules$all.class which isn't reproducible
On 21/10/2021 10:49, Jaikiran Pai wrote: : Digging into it, it appears that since the ModuleDescriptor#equals() calls equals() on enum types (in this specific case on ModuleDescriptor.Requires.Modifier) and since enum type equality is implemented as identity checks, those identity checks are surprisingly failing. More specifically ModuleDescriptor.Requires.Modifier.MANDATED == ModuleDescriptor.Requires.Modifier.MANDATED is equating to false because at runtime I see that two different instances of ModuleDescriptor.Requires.Modifier.MANDATED have been loaded (by the same boot module classloader). Although I use ModuleDescriptor.Requires.Modifier.MANDATED as an example, the same is reproducible with other enum values like ModuleDescriptor.Requires.Modifier.TRANSITIVE. This appears to be specific to CDS since running the above program with: java -Xshare:off EnumEquality succeeds and the ModuleDescriptor equality check passes. In short, it looks like there is some general issue with CDS and equality checks with enums and perhaps deserves a separate JBS issue? I've asked Ioi Lam to comment on this, off-hand I'm not aware of any issues with CDS here but it may be related to the archiving of object graphs. -Alan
Re: JDK-8275509: (jlink) SystemModulesPlugin generates a jdk.internal.module.SystemModules$all.class which isn't reproducible
Hello Alan,
On 19/10/21 7:40 pm, Alan Bateman wrote:
On 19/10/2021 14:49, Jaikiran Pai wrote:
Ah! So this exact same investigation had already happened a few weeks
back then. I haven't subscribed to that list, so missed it. I see in
one of those messages this part:
"Off hand I can't think of any issues with the ModuleDescriptor
hashCode. It is computed at link time and should be deterministic. If
I were to guess then then this may be something to do with the module
version recorded at compile-time at that is one of the components
that the hash is based on."
To be clear, is the ModuleDescriptor#hashCode() expected to return
reproducible (same) hashCode across multiple runs? What currently
changes the hashCode() across multiple runs is various components
within ModuleDescriptor's hashCode() implementation using the
hashCode() of the enums (specifically the various Modifier enums).
The discussion on jigsaw-dev didn't get to the bottom of the issue at
the time, mostly because it wasn't easy to reproduce.
Now that the issue is clearer then we should fix it. Aside from
reproducible builds then I expect it is possible to use a
ModuleDescriptor.Builder to create a ModuleDescriptor that is equal to
a ModuleDescriptor in boot layer configuration but with a different
hashCode.
Based on this input, one of the tests I have included for verifying this
proposed hashCode fix, involves dealing with a boot layer
ModuleDescriptor and then verifying it's hashCode against a
ModuleDescriptor that is built for the same module using the
ModuleDescriptor.Builder. It does reproduce the hashCode issue. However,
that test seems to have exposed a different bug with CDS and equality
checks against enums (which impact ModuleDescriptor#equals()).
More precisely, consider this trivial Java code:
import java.lang.module.*;
import java.io.*;
import java.util.*;
public class EnumEquality {
public static void main(final String[] args) throws Exception {
String moduleName = "java.sql";
// load the "java.sql" module from boot layer
Optional bootModule =
ModuleLayer.boot().findModule(moduleName);
if (bootModule.isEmpty()) {
throw new RuntimeException(moduleName + " module is missing
in boot layer");
}
ModuleDescriptor m1 = bootModule.get().getDescriptor();
// now recreate the same module using the ModuleDescriptor.read
on the module's module-info.class
ModuleDescriptor m2;
try (InputStream moduleInfo =
bootModule.get().getResourceAsStream("module-info.class")) {
if (moduleInfo == null) {
throw new RuntimeException("Could not locate
module-info.class in " + moduleName + " module");
}
// this will internally use a ModuleDescriptor.Builder to
construct the ModuleDescriptor
m2 = ModuleDescriptor.read(moduleInfo);
}
if (!m1.equals(m2)) {
// root cause - the enums aren't "equal"
for (ModuleDescriptor.Requires r1 : m1.requires()) {
if (r1.name().equals("java.base")) {
for (ModuleDescriptor.Requires r2 : m2.requires()) {
if (r2.name().equals("java.base")) {
System.out.println("Modifiers r1 " +
r1.modifiers() + " r2 " + r2.modifiers()
+ " --> equals? " +
r1.modifiers().equals(r2.modifiers()));
}
}
}
}
throw new RuntimeException("ModuleDescriptor(s) aren't
equal: \n" + m1 + "\n" + m2);
}
System.out.println("Success");
}
}
This program uses "java.sql" as the module under test. This "java.sql"
is a boot layer module and among other things has:
requires transitive java.logging;
requires transitive java.transaction.xa;
requires transitive java.xml;
in its module definition[1]. The program first loads this module's
ModuleDescriptor into an instance m1, using the boot() module layer. It
then "reconstructs" this same module by reading the module-info.class of
this module, using the ModuleDescriptor.read() API (which internally
calls ModuleDescriptor.Builder). This is stored into m2. m1 and m2 are
then checked for equality (using a call to equals() method). This
equality check keeps failing consistently.
Digging into it, it appears that since the ModuleDescriptor#equals()
calls equals() on enum types (in this specific case on
ModuleDescriptor.Requires.Modifier) and since enum type equality is
implemented as identity checks, those identity checks are surprisingly
failing. More specifically ModuleDescriptor.Requires.Modifier.MANDATED
== ModuleDescriptor.Requires.Modifier.MANDATED is equating to false
because at runtime I see that two different instances of
ModuleDescriptor.Requires.Modifier.MANDATED have been loaded (by the
same boot module classloader). Although I u
Re: RFR: 8270490: Charset.forName() taking fallback default value [v2]
On Thu, 21 Oct 2021 06:50:41 GMT, Alan Bateman wrote:
>> src/java.base/share/classes/java/io/Console.java line 587:
>>
>>> 585: try {
>>> 586: cs = Charset.forName(csname, null);
>>> 587: } catch (Exception ignored) { }
>>
>> The comment which suggests this enhancement was about eliminating such
>> "exception ignored" code paths. Is it still needed here? And if it is needed
>> why do we pass the null as a fallback?
>
> Right, I think both try-catch usages will be removed.
Apparently `IllegalCharsetNameException` or `IllegalArgumentException` could
still be thrown - so removing the `try-catch` would be a change of behaviour in
those cases. It all depends on whether there is a chance that these exceptions
could be thrown in this particular context (with these particular input
parameters) - which I am not able to tell - but maybe someone more familiar
with this code could...
-
PR: https://git.openjdk.java.net/jdk/pull/6045
Re: RFR: 8250678: ModuleDescriptor.Version parsing treats empty segments inconsistently
On Mon, 27 Sep 2021 08:22:02 GMT, Alan Bateman wrote: >> Could you please review the 8250678 bug fixes? >> >> The `parse` method of ModuleDescriptor.Version class behaves incorrectly >> when the input string contains consecutive delimiters. >> >> The `parse` method treats strings as three sections, but the parsing method >> differs between the method for the version sections and the ones for others. >> In version sections, the `parse` method takes a single character in a loop >> and determines whether it is a delimiter. In pre and build sections, the >> parse method takes two characters in a loop and determines whether the >> second character is the delimiter. Therefore, if the string contains a >> sequence of delimiters in pre or build section, the `parse` method treats >> character at the odd-numbered position as a token and the one at >> even-numbered as a delimiter. >> >> A string containing consecutive delimiters is an incorrect version string, >> but this behavior does not follow the API specification. >> https://download.java.net/java/early_access/jdk18/docs/api/java.base/java/lang/module/ModuleDescriptor.Version.html >> >> Therefore, I propose to fix the parsing method of pre and build section in >> the same way as the version. > > I think this is okay, just maybe unusual to have repeated punctuation > creating the case where a component is empty. @mbreinhold may wish to comment > on this PR. @AlanBateman I have been waiting for a reply from @mbreinhold , but I haven't received it yet. I would like to contribute this PR as soon as possible. Do you have any ideas on how to do it? - PR: https://git.openjdk.java.net/jdk/pull/5679
