RFR: 8258117: jar tool sets the time stamp of module-info.class entries to the current time
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. - Commit messages: - 8258117: jar tool sets the time stamp of module-info.class entries to the current time Changes: https://git.openjdk.java.net/jdk/pull/5486/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=5486=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8258117 Stats: 317 lines in 2 files changed: 298 ins; 0 del; 19 mod Patch: https://git.openjdk.java.net/jdk/pull/5486.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5486/head:pull/5486 PR: https://git.openjdk.java.net/jdk/pull/5486
Re: RFR: 8273514: java/util/DoubleStreamSums/CompensatedSums.java failure [v2]
On Thu, 9 Sep 2021 16:30:29 GMT, Ian Graves wrote:
>> Relaxing some assertion bounds to allow for small error values that still
>> show improvement over previous summation method.
>
> Ian Graves has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Tweaking asserts
Marked as reviewed by darcy (Reviewer).
test/jdk/java/util/DoubleStreamSums/CompensatedSums.java line 87:
> 85: badParallelStreamError +=
> Math.pow(computeFinalSum(DoubleStream.of(rand).parallel().collect(doubleSupplier,objDoubleConsumer,badCollectorConsumer))
> - sum[0], 2);
> 86:
> 87:
Above, if there are going to be multiple uese of Math.pow(arg, 2) in the test,
I recommend defining a local private static square(double d) method for this
test where square is
double square(double arg) {return arg * arg;}
The library method pow(arg, 2) does a check for the exponent being two and does
a multiple instead.
-
PR: https://git.openjdk.java.net/jdk/pull/5430
Re: RFR: 8231640: (prop) Canonical property storage [v11]
On Sun, 12 Sep 2021 14:03:42 GMT, Jaikiran Pai wrote: >> The commit in this PR implements the proposal for enhancement that was >> discussed in the core-libs-dev mailing list recently[1], for >> https://bugs.openjdk.java.net/browse/JDK-8231640 >> >> At a high level - the `store()` APIs in `Properties` have been modified to >> now look for the `SOURCE_DATE_EPOCH` environment variable[2]. If that env >> variable is set, then instead of writing out the current date time as a date >> comment, the `store()` APIs instead will use the value set for this env >> variable to parse it to a `Date` and write out the string form of such a >> date. The implementation here uses the `d MMM HH:mm:ss 'GMT'` date >> format and `Locale.ROOT` to format and write out such a date. This should >> provide reproducibility whenever the `SOURCE_DATE_EPOCH` is set. >> Furthermore, intentionally, no changes in the date format of the "current >> date" have been done. >> >> These modified `store()` APIs work in the presence of the `SecurityManager` >> too. The caller is expected to have a read permission on the >> `SOURCE_DATE_EPOCH` environment variable. If the caller doesn't have that >> permission, then the implementation of these `store()` APIs will write out >> the "current date" and will ignore any value that has been set for the >> `SOURCE_DATE_EPOCH` env variable. This should allow for backward >> compatibility of existing applications, where, when they run under a >> `SecurityManager` and perhaps with an existing restrictive policy file, the >> presence of `SOURCE_DATE_EPOCH` shouldn't impact their calls to the >> `store()` APIs. >> >> The modified `store()` APIs will also ignore any value for >> `SOURCE_DATE_EPOCH` that cannot be parsed to an `long` value. In such >> cases, the `store()` APIs will write out the "current date" and ignore the >> value set for this environment variable. No exceptions will be thrown for >> such invalid values. This is an additional backward compatibility precaution >> to prevent any rogue value for `SOURCE_DATE_EPOCH` from breaking >> applications. >> >> An additional change in the implementation of these `store()` APIs and >> unrelated to the date comment, is that these APIs will now write out the >> property keys in a deterministic order. The keys will be written out in the >> natural ordering as specified by `java.lang.String#compareTo()` API. >> >> The combination of the ordering of the property keys when written out and >> the usage of `SOURCE_DATE_EPOCH` environment value to determine the date >> comment should together allow for reproducibility of the output generated by >> these `store()` APIs. >> >> New jtreg test classes have been introduced to verify these changes. The >> primary focus of `PropertiesStoreTest` is the ordering aspects of the >> property keys that are written out. On the other hand >> `StoreReproducibilityTest` focuses on the reproducibility of the output >> generated by these APIs. The `StoreReproducibilityTest` runs these tests >> both in the presence and absence of `SecurityManager`. Plus, in the presence >> of SecurityManager, it tests both the scenarios where the caller is granted >> the requisite permission and in other case not granted that permission. >> >> These new tests and existing tests under `test/jdk/java/util/Properties/` >> pass with these changes. >> >> [1] >> https://mail.openjdk.java.net/pipermail/core-libs-dev/2021-August/080758.html >> [2] https://reproducible-builds.org/specs/source-date-epoch/ > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > Update javadoc based on Stuart's inputs Thinking more about this, given that there's now willingless to allow this system property to allow empty comments or even free form text and do away with backward compatibilty concerns that this will no longer be representing a date comment, I would like to propose something else. Let's take a small step back though. I think we all agree that the text of the default date comment isn't what is contentious. Instead, it's the presence of that comment itself. So instead of trying to allow users to feed some different text into this comment, why not just allow them to say whether or not they want this comment? In other words, why not consider the value of java.util.Properties.storeDate be either "true" or "false" and consider it a boolean system property? That should avoid all these complexities. - PR: https://git.openjdk.java.net/jdk/pull/5372
Re: RFR: 8233674: JarURLConnection.getJarFile throws Exception if some process is holding the file
On Fri, 10 Sep 2021 09:23:52 GMT, Masanori Yano wrote: > Could you please review the 8233674 bug fixes? > This problem is caused by the antivirus software opening the file for a short > time, so CreateFile() should be retried. This will need discussion as to whether the JDK should put in retry loops to work around interference from virus checkers. I wonder if it possible to get any input from the Microsoft engineers on whether native applications are expected to do this or whether the issue here is actually a configuration issue or problem with a specific virus checker. If we are adding a workaround to the JDK then it will require much more broader changes that does proposed so I think more analysis and discussion will be required before deciding whether it is right to do anywhere here or not. - Changes requested by alanb (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/5460
Re: RFR: 8231640: (prop) Canonical property storage [v10]
Hello Stuart,
On 10/09/21 11:12 pm, Stuart Marks wrote:
src/java.base/share/classes/java/util/Properties.java line 832:
830: * {@code #} character, the current date and time (formatted using the
831: * {@code EEE MMM dd HH:mm:ss zzz } {@link DateTimeFormatter date
format}),
832: * and a line separator as generated by the {@code Writer}.
Since this behavior is affected by a system property, and that property name is in the
standard `java.*` namespace, that should be documented here. In addition, `Writer` has no
notion of a line separator; it just comes from `System.lineSeparator`. I'd suggest
something like this, replacing the paragraph starting with "Next":
If the {@systemProperty java.util.Properties.storeDate} is set and is non-blank
(as determined by
{@link String#isBlank String.isBlank}, a comment line is written as follows.
First, a {@code #} character is written, followed by the contents of the
property, followed by a line separator.
If the system property is not set or is blank, a comment line is written as
follows. First, a {@code #} character is written, followed by the current date
and time formatted as if by {@link DateTimeFormatter} with the format
{@code "EEE MMM dd HH:mm:ss zzz "}, followed by a line separator.
Done. I've updated the PR to use this text in the javadoc.
This was discussed elsewhere, but after writing that, I'm now thinking that we
**should** honor the property even if it is blank. It would be useful to
disable the timestamp simply by setting the property to the empty string; this
will simply result in an empty comment line. This would simplify the code (and
the spec) by removing conditions related to String::isBlank.
OK. I don't see any obvious issues with interpreting
empty/whitespace-only value for the system property to translate to an
empty comment line. To be clear, an empty comment line that gets written
out in such cases is *always* going to be the "#" character followed by
a line separator right? Irrespective of what or how many whitespace
characters are present in the property value? Or do you want those
characters to be carried over into that comment line that gets written
out? The reason I ask this is because I think we should always write
just the "#" followed by the line separator character in such cases,
which effectively means we will still need the String::isBlank check
which would then look something like:
if (propVal.isBlank()) {
bw.write("#");
bw.newLine();
}
Side question: do we want to do any escaping or vetting of the property value?
One of the reasons why we didn't want to use the date in epoch seconds
or a formatted date string was to avoid the complexities that come with
managing that property value. So a free-form property value seemed more
appropriate and I think a free-form value still seems appropriate, but I
think we should keep any vetting to minimal. More details below.
If for example it contains embedded newlines, this could result in a malformed
property file. Or, if carefully constructed, it could contain additional
property values. I think this is an unintended consequence of changing the
property value from a numeric time value to a free-form string. We may want to
reconsider this.
The specification on the load(Reader reader) method of the
java.util.Properties class has this to say about comment lines (and
lines in general).
(snipped relevant content):
There are two kinds of line, natural lines and logical
lines.
A natural line is defined as a line of characters that is terminated
either
by a set of line terminator characters ({@code \n} or {@code \r} or
{@code \r\n}) or by the end of the stream. A natural line may be
either a blank line, a comment line, or hold all or some of a
key-element pair. A logical
line holds all the data of a key-element pair, which may be spread
out across several adjacent natural lines by escaping
the line terminator sequence with a backslash character
{@code \}. Note that a comment line cannot be extended
in this manner; every natural line that is a comment must have
its own comment indicator, as described below.
With that knowledge about comment lines, I think what we should do is
disallow system property values that contain any form of line terminator
characters (\n or \r or \r\n). If the system property value is _not_
blank (as specified by String::isBlank) and contains any of these line
terminator characters, we should simply ignore it and write out the
current date as the date comment. That, IMO, should prevent any of these
sneaky/rogue value that can end up either creating additional property
key/values to be written out or causing any malformed properties file.
Plus, that would help us keep the vetting to minimal without involving
too much complexity.
src/java.base/share/classes/java/util/Properties.java line 855:
853: * the value of this system property
Re: RFR: 8231640: (prop) Canonical property storage [v11]
> The commit in this PR implements the proposal for enhancement that was > discussed in the core-libs-dev mailing list recently[1], for > https://bugs.openjdk.java.net/browse/JDK-8231640 > > At a high level - the `store()` APIs in `Properties` have been modified to > now look for the `SOURCE_DATE_EPOCH` environment variable[2]. If that env > variable is set, then instead of writing out the current date time as a date > comment, the `store()` APIs instead will use the value set for this env > variable to parse it to a `Date` and write out the string form of such a > date. The implementation here uses the `d MMM HH:mm:ss 'GMT'` date > format and `Locale.ROOT` to format and write out such a date. This should > provide reproducibility whenever the `SOURCE_DATE_EPOCH` is set. Furthermore, > intentionally, no changes in the date format of the "current date" have been > done. > > These modified `store()` APIs work in the presence of the `SecurityManager` > too. The caller is expected to have a read permission on the > `SOURCE_DATE_EPOCH` environment variable. If the caller doesn't have that > permission, then the implementation of these `store()` APIs will write out > the "current date" and will ignore any value that has been set for the > `SOURCE_DATE_EPOCH` env variable. This should allow for backward > compatibility of existing applications, where, when they run under a > `SecurityManager` and perhaps with an existing restrictive policy file, the > presence of `SOURCE_DATE_EPOCH` shouldn't impact their calls to the `store()` > APIs. > > The modified `store()` APIs will also ignore any value for > `SOURCE_DATE_EPOCH` that cannot be parsed to an `long` value. In such cases, > the `store()` APIs will write out the "current date" and ignore the value set > for this environment variable. No exceptions will be thrown for such invalid > values. This is an additional backward compatibility precaution to prevent > any rogue value for `SOURCE_DATE_EPOCH` from breaking applications. > > An additional change in the implementation of these `store()` APIs and > unrelated to the date comment, is that these APIs will now write out the > property keys in a deterministic order. The keys will be written out in the > natural ordering as specified by `java.lang.String#compareTo()` API. > > The combination of the ordering of the property keys when written out and the > usage of `SOURCE_DATE_EPOCH` environment value to determine the date comment > should together allow for reproducibility of the output generated by these > `store()` APIs. > > New jtreg test classes have been introduced to verify these changes. The > primary focus of `PropertiesStoreTest` is the ordering aspects of the > property keys that are written out. On the other hand > `StoreReproducibilityTest` focuses on the reproducibility of the output > generated by these APIs. The `StoreReproducibilityTest` runs these tests > both in the presence and absence of `SecurityManager`. Plus, in the presence > of SecurityManager, it tests both the scenarios where the caller is granted > the requisite permission and in other case not granted that permission. > > These new tests and existing tests under `test/jdk/java/util/Properties/` > pass with these changes. > > [1] > https://mail.openjdk.java.net/pipermail/core-libs-dev/2021-August/080758.html > [2] https://reproducible-builds.org/specs/source-date-epoch/ Jaikiran Pai has updated the pull request incrementally with one additional commit since the last revision: Update javadoc based on Stuart's inputs - Changes: - all: https://git.openjdk.java.net/jdk/pull/5372/files - new: https://git.openjdk.java.net/jdk/pull/5372/files/1d24a3a8..c1dfb18f Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5372=10 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5372=09-10 Stats: 17 lines in 1 file changed: 6 ins; 3 del; 8 mod Patch: https://git.openjdk.java.net/jdk/pull/5372.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5372/head:pull/5372 PR: https://git.openjdk.java.net/jdk/pull/5372
Integrated: 8273484: Cleanup unnecessary null comparison before instanceof check in java.naming
On Mon, 6 Sep 2021 08:08:33 GMT, Andrey Turbanov wrote: > Update code checks both non-null and instance of a class in java.naming > module classes. > The checks and explicit casts could also be replaced with pattern matching > for the instanceof operator. > For example: > The following code: > > return (obj != null && > obj instanceof CompoundName && > impl.equals(((CompoundName)obj).impl)); > > > Can be simplified to: > > > return (obj instanceof CompoundName other) && > impl.equals(other.impl); > > > See similar cleanup in java.base - > https://bugs.openjdk.java.net/browse/JDK-8258422 This pull request has now been integrated. Changeset: 2ee1f96c Author:Andrey Turbanov Committer: Aleksei Efimov URL: https://git.openjdk.java.net/jdk/commit/2ee1f96c14b80b63a29445629b1f2e1caf88e075 Stats: 44 lines in 13 files changed: 0 ins; 11 del; 33 mod 8273484: Cleanup unnecessary null comparison before instanceof check in java.naming Reviewed-by: aefimov, dfuchs - PR: https://git.openjdk.java.net/jdk/pull/5374
Re: RFR: 8273541: Cleaner Thread creates with normal priority instead of MAX_PRIORITY - 2
On 11/09/2021 5:24 am, kabutz wrote: On Fri, 10 Sep 2021 14:08:05 GMT, Claes Redestad wrote: This looks good. @cl4es might want to confirm that change was not intentional. Completely unintentional, and perplexing since it's not a simple copy-paste error.. The thread priorities in Java are the wrong way round, (high is 10, low is 1) so I was not surprised by this mistake. They are not "the wrong way around" they followed Solaris Global Dispatch Priorities (and Windows priorities for that matter). It makes a lot more sense than systems where a "higher priority" means a lower numerical priority value IMO. :) Cheers, David - PR: https://git.openjdk.java.net/jdk/pull/5439
