Re: RFR: 8258117: jar tool sets the time stamp of module-info.class entries to the current time [v2]
On Fri, 17 Sep 2021 12:54:07 GMT, Jaikiran Pai wrote: >> The commit here is a potential fix for the issue noted in >> https://bugs.openjdk.java.net/browse/JDK-8258117. >> >> The change here repurposes an existing internal interface `ModuleInfoEntry` >> to keep track of the last modified timestamp of a `module-info.class` >> descriptor. >> >> This commit uses the timestamp of the `module-info.class` on the filesystem >> to set the time on the `JarEntry`. There are a couple of cases to consider >> here: >> >> 1. When creating a jar (using `--create`), we use the source >> `module-info.class` from the filesystem and then add extended info to it >> (attributes like packages, module version etc...). In such cases, this patch >> will use the lastmodified timestamp from the filesystem of >> `module-info.class` even though we might end up updating/extending/modifying >> (for example by adding a module version) its content while storing it as a >> `JarEntry`. >> >> 2. When updating a jar (using `--update`), this patch will use the >> lastmodified timestamp of `module-info.class` either from the filesystem or >> from the source jar's entry (depending on whether a new `module-info.class` >> is being passed to the command). Here too, it's possible that we might end >> up changing/modifying/extending the `module-info.class` (for example, >> changing the module version to a new version) that gets written into the >> updated jar file, but this patch _won't_ use `System.currentTimeMillis()` >> even in such cases. >> >> If we do have to track actual changes that might happen to >> `module-info.class` while extending its info (in `extendedInfoBytes()`) and >> then decide whether to use current system time as last modified time, then >> this will require a bigger change and also a discussion on what kind of >> extending of module-info.class content will require a change to the >> lastmodifiedtime of that entry. > > Jaikiran Pai 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 two additional > commits since the last revision: > > - Merge latest from master branch > - 8258117: jar tool sets the time stamp of module-info.class entries to the > current time Keep alive. - PR: https://git.openjdk.java.net/jdk/pull/5486
Re: RFR: 8270490: Charset.forName() taking fallback default value [v3]
On Sat, 23 Oct 2021 19:29:30 GMT, Sergey Bylokhov wrote:
> Just an idea, should we check that the second parameter is null or not? Any
> pros and cons of that? For example should this code be allowed:
>
> ```
> var cs = Charset.forName(charsetName, null);
> if (cs == null) {
>System.err.println("Used UTF-8 encoding instead of "+charsetName+");
>cs = StandardCharsets.UTF_8;
> }
> ```
Yes, that's the whole purpose of allowing `null` for `fallback`.
> src/java.base/share/classes/java/io/Console.java line 589:
>
>> 587: }
>> 588: if (cs == null) {
>> 589: cs = Charset.forName(StaticProperty.nativeEncoding(),
>> Charset.defaultCharset());
>
> Not sure but looks like this class tries to maintain 80 chars per line rule?
Fixed.
-
PR: https://git.openjdk.java.net/jdk/pull/6045
Re: RFR: 8270490: Charset.forName() taking fallback default value [v4]
> 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: Reflecting review comments - Changes: - all: https://git.openjdk.java.net/jdk/pull/6045/files - new: https://git.openjdk.java.net/jdk/pull/6045/files/7c73c5ba..0e787e7a Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=6045=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=6045=02-03 Stats: 7 lines in 2 files changed: 2 ins; 0 del; 5 mod Patch: https://git.openjdk.java.net/jdk/pull/6045.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6045/head:pull/6045 PR: https://git.openjdk.java.net/jdk/pull/6045
Re: RFR: 8270490: Charset.forName() taking fallback default value [v3]
On Sat, 23 Oct 2021 06:42:52 GMT, Alan Bateman wrote: > I think you'll need to adjust the description make it clear that "fallback" > is used when the input is not a legal charset name or the charset is not > available. Made the method description clearer. - PR: https://git.openjdk.java.net/jdk/pull/6045
Re: RFR: 8153490: Cannot setBytes() if incoming buffer's length is bigger than number of elements we want to insert. [v5]
On Fri, 15 Oct 2021 09:30:18 GMT, Mitsuru Kariya wrote: >> Fix `SerialBlob.setBytes(long pos, byte[] bytes, int offset, int length)` in >> the following cases: >> >> 1. `pos - 1 + bytes.length - offset > this.length() && pos - 1 + length <= >> this.length()` >>The original implementation throws `ArrayIndexOutOfBoundsException` but >> this case should end successfully. >>(test31) >> >> 2. `pos - 1 + length > this.length()` >>The original implementation throws `ArrayIndexOutOfBoundsException` but >> this case should end successfully. *1 >>(test32) >> >> 3. `pos == this.length() + 1` >>The original implementation throws `SerialException` but this case should >> end successfully. *2 >>(test33) >> >> 4. `length < 0` >>The original implementation throws `ArrayIndexOutOfBoundsException` but >> this case should throw `SerialException`. >>(test34) >> >> 5. `offset + length > Integer.MAX_VALUE` >>The original implementation throws `ArrayIndexOutOfBoundsException` (or >> `OutOfMemoryError` in most cases) but this case should throw >> `SerialException`. >>(test35) >> >> Additionally, fix `SerialClob.setString(long pos, String str, int offset, >> int length)` in the following cases: >> >> 1. `offset > str.length()` >>The original implementaion throws `StringIndexOutOfBoundsException` but >> this case should throw `SerialException`. >>(test39) >> >> 2. `pos - 1 + str.length() - offset > this.length() && pos - 1 + length <= >> this.length()` >>The original implementation throws `ArrayIndexOutOfBoundsException` but >> this case should end successfully. >>(test32) >> >> 3. `pos - 1 + length > this.length()` >>The original implementaion throws `SerialException` but this case should >> end successfully. *3 >>(test40) >> >> 4. `pos == this.length() + 1` >>The original implementaion throws `SerialException` but this case should >> end successfully. *4 >>(test41) >> >> 5. `length < 0` >>The original implementation throws `StringIndexOutOfBoundsException` but >> this case should throw `SerialException`. >>(test42) >> >> 6. `offset + length > Integer.MAX_VALUE` >>The original implementation throws `ArrayIndexOutOfBoundsException` (or >> `OutOfMemoryError` in most cases) but this case should throw >> `SerialException`. >>(test43) >> >> >> The javadoc has also been modified according to the above. >> >> *1 The documentation of `Blob.setBytes()` says, "If the end of the Blob >> value is reached while writing the array of bytes, then the length of the >> Blob value will be increased to accommodate the extra bytes." >> >> *2 The documentation of `Blob.setBytes()` says, "If the value specified for >> pos is greater than the length+1 of the BLOB value then the behavior is >> undefined." >>So, it should work correctly when pos == length+1 of the BLOB value. >> >> *3 The documentation of `Clob.setString()` says, "If the end of the Clob >> value is eached while writing the given string, then the length of the Clob >> value will be increased to accommodate the extra characters." >> >> *4 The documentation of `Clob.setString()` says, "If the value specified for >> pos is greater than the length+1 of the CLOB value then the behavior is >> undefined." >>So, it should work correctly when pos == length+1 of the CLOB value. > > Mitsuru Kariya has updated the pull request incrementally with one additional > commit since the last revision: > > Follow the comment Any chance you can sync up your workspace with master? I am getting errors when I fetch and try to run our internal tests. - PR: https://git.openjdk.java.net/jdk/pull/4001
Integrated: 8275293: A change done with JDK-8268764 mismatches the java.rmi.server.ObjID.hashCode spec
On Fri, 15 Oct 2021 07:17:52 GMT, Сергей Цыпанов wrote: > It looks like we cannot use `Long.hashCode(long)` for > `java.rmi.server.ObjID.hashCode()` due to specification: > https://docs.oracle.com/en/java/javase/17/docs/api/java.rmi/java/rmi/server/ObjID.html#hashCode(). This pull request has now been integrated. Changeset: 5bbe4cae Author:Sergey Tsypanov Committer: Stuart Marks URL: https://git.openjdk.java.net/jdk/commit/5bbe4cae8746765d2ce965b06fd1e5cf512326ae Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod 8275293: A change done with JDK-8268764 mismatches the java.rmi.server.ObjID.hashCode spec Reviewed-by: rriggs, smarks - PR: https://git.openjdk.java.net/jdk/pull/5963
Re: RFR: 8275293: A change done with JDK-8268764 mismatches the java.rmi.server.ObjID.hashCode spec
On Fri, 15 Oct 2021 07:17:52 GMT, Сергей Цыпанов wrote: > It looks like we cannot use `Long.hashCode(long)` for > `java.rmi.server.ObjID.hashCode()` due to specification: > https://docs.oracle.com/en/java/javase/17/docs/api/java.rmi/java/rmi/server/ObjID.html#hashCode(). Marked as reviewed by smarks (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/5963
Re: RFR: 8270490: Charset.forName() taking fallback default value [v3]
On Fri, 22 Oct 2021 17:33:43 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:
>
> Removed `@throws IllegalCharsetNameException`
Just an idea, should we check that the second parameter is null or not? Any
pros and cons of that?
For example should this code be allowed:
var cs = Charset.forName(charsetName, null);
if (cs == null) {
System.err.println("Used UTF-8 encoding instead of "+charsetName+");
cs = StandardCharsets.UTF_8;
}
Or something like this should be forced:
var cs = Charset.forName(charsetName, fallback);
if (cs == fallback) {
System.err.println("Used UTF-8 encoding instead of "+charsetName+");
}
I have no preference.
-
PR: https://git.openjdk.java.net/jdk/pull/6045
Re: RFR: 8268595: java/io/Serializable/serialFilter/GlobalFilterTest.java#id1 failed in timeout
On Tue, 19 Oct 2021 13:00:01 GMT, Jaikiran Pai wrote: > The `GlobalFilterTest` has to 2 `@test` tags. One of them has failed with a > timeout as noted in https://bugs.openjdk.java.net/browse/JDK-8268595. The > timeout seems to have happened even after the tests had already completed > successfully. Like I note in the JBS comments of that issue, I suspect it > might have to do with the > "-XX:StartFlightRecording:name=DeserializationEvent,dumponexit=true" usage in > this test action. > > The commit in this PR removes that second `@test` altogether because (correct > me if I'm wrong) from what I understand, this test never enables the > DeserializationEvent which means there is no JFR events being captured for > deserialization in this test, nor does the test do any JFR events related > testing. So, I think this second `@test` is virtually a no-op when it comes > to the JFR testing. There's a separate `TestDeserializationEvent` which has a > comprehensive testing of the DeserializationEvent. Marked as reviewed by chegar (Reviewer). I agree that the second `@test` is not all that useful, and can probably be just removed. If I remember correctly, I added the second `@test` during development of the feature before the more comprehensive test was added, and also when experimenting with the event being enabled by default, but it is not adding much value now. - PR: https://git.openjdk.java.net/jdk/pull/6008
Incorrect assumption on the class name in compile(InputSource input, String name) at XSLTC.java
Hi there,
The code in compile(InputSource input, String name) at
https://github.com/openjdk/jdk/blob/master/src/java.xml/share/classes/com/sun/org/apache/xalan/internal/xsltc/compiler/XSLTC.java
seems incorrect as follows:
public boolean compile(InputSource input, String name) {
try {
// Reset globals in case we're called by compile(ArrayList v);
reset();
// The systemId may not be set, so we'll have to check the URL
String systemId = null;
if (input != null) {
systemId = input.getSystemId();
}
// Set the translet class name if not already set
if (_className == null) {
if (name != null) {
setClassName(name);
}
else if (systemId != null && !systemId.equals("")) {
setClassName(Util.baseName(systemId)); <---
incorrect here
}
// Ensure we have a non-empty class name at this point
if (_className == null || _className.length() == 0) {
setClassName("GregorSamsa"); // default translet name
}
}
...
Specifically, the code above assumes that systemId is something like
"xxx:yyy" in which case the class name (_className) is
"die.verwandlung.yyy" ("die.verwandlung." is the default package name since
Java11) However,Util.baseName(systemId) returns null when systemId is
something like "xxx:" (empty after ":"), in which case the class name
(_className) ends up with "die.verwandlung."(an invalid class name inserted
in the constant pool of the generated bytecode).
>From the perspective of robustness, the code above should be updated to
handle the situation. e.g.
else if (systemId != null && !systemId.equals("")) {
String baseName = Util.baseName(systemId);
if ((baseName != null) && (baseName.length() > 0))
{ <--
setClassName(baseName);
}
which means it should check whether the returned base name from
Util.baseName(systemId) is empty before setting the class name; otherwise,
it should use the default class name ("GregorSamsa").
Let me know if any other concern on this fix.
Thanks and Best Regards
Cheng Jin
Re: RFR: 8244202: Implementation of JEP 418: Internet-Address Resolution SPI [v5]
On Wed, 20 Oct 2021 18:25:24 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/spi/InetAddressResolverProvider.java
> line 52:
>
>> 50: /**
>> 51: * Initialise and return the {@link InetAddressResolver} provided by
>> 52: * this provider. This method is called by {@link InetAddress} when
>
> "the InetAddressResolver" suggests there is just one instance. That is
> probably the case but I don't think you want to be restricted to that.
Initialise -> Initialize to be consistent with other usages that use US
spelling.
-
PR: https://git.openjdk.java.net/jdk/pull/5822
Re: RFR: 8244202: Implementation of JEP 418: Internet-Address Resolution SPI [v7]
On Fri, 22 Oct 2021 14:27:41 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:
>
> More javadoc updates to API classes
I've done another pass over the API. Overall it's looking very good and my
comments are just to improve the javadoc in a few places to make it clearer.
src/java.base/share/classes/java/net/InetAddress.java line 169:
> 167: * Access Protocol (LDAP).
> 168: * The particular naming services that the built-in resolver uses by
> default
> 169: * depend on the configuration of the local machine.
depend -> depends
src/java.base/share/classes/java/net/spi/InetAddressResolver.java line 38:
> 36: * deployed as a href="InetAddressResolverProvider.html#system-wide-resolver">
> 37: * system-wide resolver. {@link InetAddress} delegates all lookup
> requests to
> 38: * the deployed system-wide resolver instance.
I think the class description would be a bit clearer if we drop sentence 2
because it overlaps with paragraph 2. I think we can adjust sentence 3 to
"InetAddress delegates all lookup operations to the system-wide resolver" and
it will all flow nicely.
src/java.base/share/classes/java/net/spi/InetAddressResolver.java line 88:
> 86: * to a valid IP address length
> 87: */
> 88: String lookupByAddress(byte[] addr) throws UnknownHostException;
I assume this throws NPE if addr is null.
src/java.base/share/classes/java/net/spi/InetAddressResolver.java line 137:
> 135: /**
> 136: * This factory method creates {@link LookupPolicy LookupPolicy}
> instance with a provided
> 137: * {@code characteristics} value.
This should be "This factory method creates a LookupPolicy ...".
src/java.base/share/classes/java/net/spi/InetAddressResolverProvider.java line
45:
> 43: * system-wide resolver instance, which is used by
> 44: * href="{@docRoot}/java.base/java/net/InetAddress.html#host-name-resolution">
> 45: * InetAddress.
I think we should prune some some of the text from the class description to
avoid repetition. Here's a suggestion:
1. Add the following immediately after the sentence "A given innovation ..."
"It is set after the VM is fully initialized and when an invocation of a method
in InetAddress class triggers the first lookup operation.
2. Replace the text in L47-65 with:
"A resolver provider is located and loaded by InetAddress to create the
systwm-wide resolver as follows:
3. Replace the remaining three usages of "install" with "set".
-
PR: https://git.openjdk.java.net/jdk/pull/5822
Re: RFR: 8270490: Charset.forName() taking fallback default value [v3]
On Fri, 22 Oct 2021 17:33:43 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: > > Removed `@throws IllegalCharsetNameException` I think you'll need to adjust the description make it clear that "fallback" is used when the input is not a legal charset name or the charset is not available. - PR: https://git.openjdk.java.net/jdk/pull/6045
Re: RFR: 8275512: Upgrade required version of jtreg to 6.1 [v2]
On Wed, 20 Oct 2021 09:28:30 GMT, Leslie Zhai wrote: > requires jtreg version 6.1 b1 or higher This confused me a bit too; I was using jtreg-6+1.tar.gz from [Adoption Group build](https://ci.adoptopenjdk.net/view/Dependencies/job/dependency_pipeline/lastSuccessfulBuild/artifact/jtreg/), and apparently 6+1 is 6.0b1, not 6.1. For now I'm using jtregtip. - PR: https://git.openjdk.java.net/jdk/pull/6012
