Re: RFR: 8231640: (prop) Canonical property storage [v22]
On Wed, 22 Sep 2021 10:27:45 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 two additional > commits since the last revision: > > - Revert "Implement Mark's suggestion in CSR to include the > java.properties.date in the list of system properties listed in > System::getProperties()" > >Additional inputs since this specific commit was introduced have leaned > towards not listing this property in System::getProperties() > >This reverts commit 458c1fd37cf1e8e8bd0a60a171d173ce5da12b7c. > - reduce the line length of code comment The CSR for this has been approved and there are no pending changes in this PR. I am thinking of integrating this PR in around 24 hours from now if this looks good. - PR: https://git.openjdk.java.net/jdk/pull/5372
Re: Discussion: easier Stream closing
On Sun, Sep 26, 2021 at 6:13 PM Remi Forax wrote:
>
> > List list =
> > Files.list(Path.of("/etc")).map(Path::getFileName).consumeAndClose(Stream::toList);
> >
> > What do you think?
>
> This one does not work because if Path::getFileName fails with an exception,
> close() will not be called.
Well, as Path::getFileName is not actually executed here, only linked,
there are only a few chances
when map(Path::getFileName) may fail. Probably StackOverflowError,
OutOfMemoryError, or LinkageError.
Probably not so important for properly functioning application. I
mean, if something of these happens, leaking
file descriptor would be a lesser problem.
With best regards,
Tagir Valeev.
> What you are proposing here is equivalent to
> try(var stream = Files.list(Path.of("/etc")).map(Path::getFileName)) {
> stream.toList()
> }
>
> instead of
> try(var stream = Files.list(Path.of("/etc"))) {
> stream..map(Path::getFileName).toList()
> }
>
> regards,
> Rémi
>
> >
> >
> > [1]
> > https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/lang/StackWalker.html#walk(java.util.function.Function)
Withdrawn: 6957241: ClassLoader.getResources() returns only 1 instance when using jar indexing
On Tue, 31 Aug 2021 12:11:46 GMT, wxiang
wrote:
> Using jarIndex for Hibench, there is an unexpected behavior with the
> exception "Exception in thread "main"
> org.apache.hadoop.fs.UnsupportedFileSystemException: No FileSystem for scheme
> "hdfs"".
>
> After investigating it, it is related to the usage of ServiceLoader with
> JarIndex.
> The below stack shows the issue with JDK11:
>
> getResource:1016, URLClassPath$JarLoader (jdk.internal.loader)
> getResource:937, URLClassPath$JarLoader (jdk.internal.loader)
> findResource:912, URLClassPath$JarLoader (jdk.internal.loader)
> next:341, URLClassPath$1 (jdk.internal.loader)
> hasMoreElements:351, URLClassPath$1 (jdk.internal.loader)
> hasNext:355, BuiltinClassLoader$1 (jdk.internal.loader)
> hasMoreElements:363, BuiltinClassLoader$1 (jdk.internal.loader)
> next:3032, CompoundEnumeration (java.lang)
> hasMoreElements:3041, CompoundEnumeration (java.lang)
> nextProviderClass:1203, ServiceLoader$LazyClassPathLookupIterator (java.util)
> hasNextService:1221, ServiceLoader$LazyClassPathLookupIterator (java.util)
> hasNext:1265, ServiceLoader$LazyClassPathLookupIterator (java.util)
> hasNext:1300, ServiceLoader$2 (java.util)
> hasNext:1385, ServiceLoader$3 (java.util)
>
> The below API tries to get all the resources with the same name.
>
> public Enumeration findResources(final String name,
> final boolean check)
> ```
> After using JarIndex, URLClassPath.findResources only returns 1 URL.
> It is the same as the description in JDK-6957241.
>
> The issue still exists in JDK18.
>
> Root cause:
>
> public Enumeration findResources(final String name,
> final boolean check) {
> return new Enumeration<>() {
> private int index = 0;
> private URL url = null;
>
> private boolean next() {
> if (url != null) {
> return true;
> } else {
> Loader loader;
> while ((loader = getLoader(index++)) != null) {
> url = loader.findResource(name, check);
> if (url != null) {
> return true;
> }
> }
> return false;
> }
> }
> ...
> };
> }
>
> With the JarIndex, there is only one loader which is corresponding to the jar
> with the index due to the implementation in JarLoader.getResource(final
> String name, boolean check, Set visited).
>
> Loaders corresponding to other jar packages will not appear in this while.
> So it only returns 1 instance.
>
> To solve the issue, I change the implementation "private boolean next()".
> If the loader has index, traverse the index and get all the resource from the
> loader.
This pull request has been closed without being integrated.
-
PR: https://git.openjdk.java.net/jdk/pull/5316
Re: RFR: 6957241: ClassLoader.getResources() returns only 1 instance when using jar indexing
On Tue, 31 Aug 2021 12:11:46 GMT, wxiang
wrote:
> Using jarIndex for Hibench, there is an unexpected behavior with the
> exception "Exception in thread "main"
> org.apache.hadoop.fs.UnsupportedFileSystemException: No FileSystem for scheme
> "hdfs"".
>
> After investigating it, it is related to the usage of ServiceLoader with
> JarIndex.
> The below stack shows the issue with JDK11:
>
> getResource:1016, URLClassPath$JarLoader (jdk.internal.loader)
> getResource:937, URLClassPath$JarLoader (jdk.internal.loader)
> findResource:912, URLClassPath$JarLoader (jdk.internal.loader)
> next:341, URLClassPath$1 (jdk.internal.loader)
> hasMoreElements:351, URLClassPath$1 (jdk.internal.loader)
> hasNext:355, BuiltinClassLoader$1 (jdk.internal.loader)
> hasMoreElements:363, BuiltinClassLoader$1 (jdk.internal.loader)
> next:3032, CompoundEnumeration (java.lang)
> hasMoreElements:3041, CompoundEnumeration (java.lang)
> nextProviderClass:1203, ServiceLoader$LazyClassPathLookupIterator (java.util)
> hasNextService:1221, ServiceLoader$LazyClassPathLookupIterator (java.util)
> hasNext:1265, ServiceLoader$LazyClassPathLookupIterator (java.util)
> hasNext:1300, ServiceLoader$2 (java.util)
> hasNext:1385, ServiceLoader$3 (java.util)
>
> The below API tries to get all the resources with the same name.
>
> public Enumeration findResources(final String name,
> final boolean check)
> ```
> After using JarIndex, URLClassPath.findResources only returns 1 URL.
> It is the same as the description in JDK-6957241.
>
> The issue still exists in JDK18.
>
> Root cause:
>
> public Enumeration findResources(final String name,
> final boolean check) {
> return new Enumeration<>() {
> private int index = 0;
> private URL url = null;
>
> private boolean next() {
> if (url != null) {
> return true;
> } else {
> Loader loader;
> while ((loader = getLoader(index++)) != null) {
> url = loader.findResource(name, check);
> if (url != null) {
> return true;
> }
> }
> return false;
> }
> }
> ...
> };
> }
>
> With the JarIndex, there is only one loader which is corresponding to the jar
> with the index due to the implementation in JarLoader.getResource(final
> String name, boolean check, Set visited).
>
> Loaders corresponding to other jar packages will not appear in this while.
> So it only returns 1 instance.
>
> To solve the issue, I change the implementation "private boolean next()".
> If the loader has index, traverse the index and get all the resource from the
> loader.
JarIndex support will be deleted. close the PR.
-
PR: https://git.openjdk.java.net/jdk/pull/5316
Withdrawn: 8273401: Remove JarIndex support in URLClassPath
On Tue, 7 Sep 2021 03:34:27 GMT, wxiang wrote: > There is a bug for URLClassPath.findResources with JarIndex. > With some discussions about the bug, the current priority is to remove the > JAR index support in URLClassPath, > and don’t need to do anything to the jar tool in the short term, except just > to move JarIndex to the jdk.jartool module. > > The PR includes: > 1. remove the JarIndex support in URLClassPath > 2. move JarIndex into jdk.jartool module. This pull request has been closed without being integrated. - PR: https://git.openjdk.java.net/jdk/pull/5383
Re: RFR: 8273401: Remove JarIndex support in URLClassPath [v2]
On Wed, 8 Sep 2021 06:22:38 GMT, wxiang wrote: >> There is a bug for URLClassPath.findResources with JarIndex. >> With some discussions about the bug, the current priority is to remove the >> JAR index support in URLClassPath, >> and don’t need to do anything to the jar tool in the short term, except just >> to move JarIndex to the jdk.jartool module. >> >> The PR includes: >> 1. remove the JarIndex support in URLClassPath >> 2. move JarIndex into jdk.jartool module. > > wxiang has updated the pull request incrementally with one additional commit > since the last revision: > > Some minor changes > > Include: > 1. remove public for INDEX_NAME in JarFile > 2. recover the definition for INDEX_NAME in JarIndex > 3. mv the test case JarIndexMergeTest.java into test/jdk/java/util/jar Firstly, disable JarIndex support in URLClassPath. So close the PR. - PR: https://git.openjdk.java.net/jdk/pull/5383
Re: Discussion: easier Stream closing
On Sun, 26 Sept 2021 at 10:29, Tagir Valeev wrote:
> List list =
> Files.list(Path.of("/etc")).map(Path::getFileName).consumeAndClose(Stream::toList);
>
> What do you think?
I fully support this. We have our own utility to do this kind of
thing, as bugs with `Files` are common:
https://github.com/OpenGamma/Strata/blob/main/modules/collect/src/main/java/com/opengamma/strata/collect/io/SafeFiles.java
Stephen
Integrated: 8274293: Build failure on macOS with Xcode 13.0 as vfork is deprecated
On Fri, 24 Sep 2021 16:18:57 GMT, xpbob wrote: > remove vfork() on Darwin This pull request has now been integrated. Changeset: 252aaa92 Author:bobpengxie Committer: Jie Fu URL: https://git.openjdk.java.net/jdk/commit/252aaa9249d8979366b37d59487b5b039d923e35 Stats: 10 lines in 2 files changed: 8 ins; 0 del; 2 mod 8274293: Build failure on macOS with Xcode 13.0 as vfork is deprecated Reviewed-by: alanb, stuefe, rriggs - PR: https://git.openjdk.java.net/jdk/pull/5686
Integrated: 8273401: Disable JarIndex support in URLClassPath
On Wed, 15 Sep 2021 09:33:10 GMT, wxiang wrote: > There is a bug for URLClassPath.findResources with JarIndex. > Currently, there was agreement on dropping the support from the > URLClassLoader implementation but it was suggested that it should be disabled > for a release or two before the code is removed. > A system property can be used to re-enable JarIndex support in URLClassPath. > > The PR includes: > Disable JarIndex support in URLClassPath by default. > Add system property jdk.net.URLClassPath.enableJarIndex to re-enable JarIndex > support. This pull request has now been integrated. Changeset: 7700b254 Author:seanwxiang Committer: Hui Shi URL: https://git.openjdk.java.net/jdk/commit/7700b25460b9898060602396fed7bc590ba242b8 Stats: 12 lines in 3 files changed: 8 ins; 1 del; 3 mod 8273401: Disable JarIndex support in URLClassPath Reviewed-by: dfuchs, lancea, alanb, mchung - PR: https://git.openjdk.java.net/jdk/pull/5524
Re: Discussion: easier Stream closing
- Original Message -
> From: "Tagir Valeev"
> To: "core-libs-dev"
> Sent: Dimanche 26 Septembre 2021 11:27:58
> Subject: Discussion: easier Stream closing
> Hello!
>
> With current NIO file API, a very simple problem to get the list of
> all files in the directory requires some ceremony:
>
> List paths;
> try (Stream stream = Files.list(Path.of("/etc"))) {
>paths = stream.toList();
> }
>
> If we skip try-with-resources, we may experience OS file handles leak,
> so it's desired to keep it. Yet, it requires doing boring stuff. In
> this sense, classic new File("/etc").list() was somewhat more
> convenient (despite its awful error handling).
>
> I like how this problem is solved in StackWalker API [1]: it limits
> the lifetime of the Stream by requiring a user-specified function to
> consume it. After the function is applied, the stream is closed
> automatically. We could add a similar overload to the Files API. As an
> additional feature, we could also translate all UncheckedIOException's
> back to IOException for more uniform exception processing:
>
> /**
> * @param dir The path to the directory
> * @param function function to apply to the stream of directory files
> * @param type of the result
> * @return result of the function
> * @throws IOException if an I/O error occurs when opening the directory, or
> * UncheckedIOException is thrown by the supplied function.
> */
> public static T list(Path dir, Function, ?
> extends T> function) throws IOException {
>try (Stream stream = Files.list(dir)) {
>return function.apply(stream);
>}
>catch (UncheckedIOException exception) {
>throw exception.getCause();
>}
> }
>
> In this case, getting the List of all files in the directory will be
> as simple as
>
> List paths = Files.list(Path.of("/etc"), Stream::toList);
> This doesn't limit the flexibility. For example, if we need only file
> names instead of full paths, we can do this:
> List paths = Files.list(Path.of("/etc"), s ->
> s.map(Path::getFileName).toList());
It seems a nice addition, forgetting the try-with-resources is a very common
source of bugs in my students code.
Part of the issue is that there is no way to easily test if there are still
open descriptors during tests.
I wonder if the VM can not offer such test using a special flag
(-XX:+CheckPendingFileDescriptor) or something like that.
>
> Alternatively, we could enhance the BaseStream interface in a similar
> way. It won't allow us to translate exceptions, but could be useful
> for every stream that must be closed after consumption:
>
> // in BaseStream:
>
> /**
> * Apply a given function to this stream, then close the stream.
> * No further operation on the stream will be possible after that.
> *
> * @param function function to apply
> * @param type of the function result
> * @return result of the function
> * @see #close()
> */
> default R consumeAndClose(Function function) {
>try(@SuppressWarnings("unchecked") S s = (S) this) {
>return function.apply(s);
>}
> }
>
> The usage would be a little bit longer but still more pleasant than
> explicit try-with-resources:
>
> List list = Files.list(Path.of("/etc")).consumeAndClose(Stream::toList);
>
> On the other hand, in this case, we are free to put intermediate
> operations outside of consumeAndClose, so we won't need to nest
> everything inside the function. Only terminal operation should be
> placed inside the consumeAndClose. E.g., if we need file names only,
> like above, we can do this:
>
> List list =
> Files.list(Path.of("/etc")).map(Path::getFileName).consumeAndClose(Stream::toList);
>
> What do you think?
This one does not work because if Path::getFileName fails with an exception,
close() will not be called.
What you are proposing here is equivalent to
try(var stream = Files.list(Path.of("/etc")).map(Path::getFileName)) {
stream.toList()
}
instead of
try(var stream = Files.list(Path.of("/etc"))) {
stream..map(Path::getFileName).toList()
}
regards,
Rémi
>
>
> [1]
> https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/lang/StackWalker.html#walk(java.util.function.Function)
Discussion: easier Stream closing
Hello!
With current NIO file API, a very simple problem to get the list of
all files in the directory requires some ceremony:
List paths;
try (Stream stream = Files.list(Path.of("/etc"))) {
paths = stream.toList();
}
If we skip try-with-resources, we may experience OS file handles leak,
so it's desired to keep it. Yet, it requires doing boring stuff. In
this sense, classic new File("/etc").list() was somewhat more
convenient (despite its awful error handling).
I like how this problem is solved in StackWalker API [1]: it limits
the lifetime of the Stream by requiring a user-specified function to
consume it. After the function is applied, the stream is closed
automatically. We could add a similar overload to the Files API. As an
additional feature, we could also translate all UncheckedIOException's
back to IOException for more uniform exception processing:
/**
* @param dir The path to the directory
* @param function function to apply to the stream of directory files
* @param type of the result
* @return result of the function
* @throws IOException if an I/O error occurs when opening the directory, or
* UncheckedIOException is thrown by the supplied function.
*/
public static T list(Path dir, Function, ?
extends T> function) throws IOException {
try (Stream stream = Files.list(dir)) {
return function.apply(stream);
}
catch (UncheckedIOException exception) {
throw exception.getCause();
}
}
In this case, getting the List of all files in the directory will be
as simple as
List paths = Files.list(Path.of("/etc"), Stream::toList);
This doesn't limit the flexibility. For example, if we need only file
names instead of full paths, we can do this:
List paths = Files.list(Path.of("/etc"), s ->
s.map(Path::getFileName).toList());
Alternatively, we could enhance the BaseStream interface in a similar
way. It won't allow us to translate exceptions, but could be useful
for every stream that must be closed after consumption:
// in BaseStream:
/**
* Apply a given function to this stream, then close the stream.
* No further operation on the stream will be possible after that.
*
* @param function function to apply
* @param type of the function result
* @return result of the function
* @see #close()
*/
default R consumeAndClose(Function function) {
try(@SuppressWarnings("unchecked") S s = (S) this) {
return function.apply(s);
}
}
The usage would be a little bit longer but still more pleasant than
explicit try-with-resources:
List list = Files.list(Path.of("/etc")).consumeAndClose(Stream::toList);
On the other hand, in this case, we are free to put intermediate
operations outside of consumeAndClose, so we won't need to nest
everything inside the function. Only terminal operation should be
placed inside the consumeAndClose. E.g., if we need file names only,
like above, we can do this:
List list =
Files.list(Path.of("/etc")).map(Path::getFileName).consumeAndClose(Stream::toList);
What do you think?
[1]
https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/lang/StackWalker.html#walk(java.util.function.Function)
