Source file launcher - Handling of pre-compiled classes of the source file

[Jaikiran Pai]

Please consider this trivial code C.java:

public class C {
    public static void main(String[] args) throws Exception {
        System.out.println("main() execution started");
    }
}


> ls
C.java

Similar to a previous discussion[1] while doing random testing, I ended
up compiling C.java explicitly using javac:

> javac C.java
> ls
C.java C.class

and then at a later date tried to use the source file launcher feature
of Java 11 (without realizing C.class was already present in the dir):

> java C.java

This threw the error:

error: class found on application class path: C

Although the error isn't that clear for the reason I note in [2], having
run into this before, I was aware what this meant and deleted the
C.class and moved on. The important part here is that the source
launcher noticed this condition and aborted even before it auto
compiled(?) and launched and executed the main() of the program.

Now consider a slight modification to that source file:

public class C {
    public static void main(String[] args) throws Exception {
        System.out.println("main() execution started");
        final B b = new B();
        System.out.println("Done");
    }

    private static class B {

    }
}

Again at some point I compiled this explicitly using javac, so my
directory is (among other things):

> ls
C$B.class    C.class        C.java

Then ran the source file launcher feature:

> java C.java
error: class found on application class path: C

As expected, ran into the same previous error. As before, in order to
move on, deleted C.class:

> rm C.class

but forgot to delete the nested static class that belonged to it. So the
directory now contained:

> ls
C$B.class    C.java

Now used the source launcher feature again:

> java C.java

This time it failed with:

main() execution started
Exception in thread "main" java.lang.IllegalAccessError: failed to
access class C$B from class C (C$B is in unnamed module of loader 'app';
C is in unnamed module of loader
com.sun.tools.javac.launcher.Main$MemoryClassLoader @1b1473ab)
    at C.main(C.java:4)

The error message isn't clear to pinpoint the issue, but at least the
reference to MemoryClassLoader was a hint that was enough for me to
understand where to look. It did take me a few minutes to realize that
C$B.class was lying around which I needed to remove too.

However, IMO, the important part here is that unlike in the first case
where the program itself wasn't launched and instead was aborted early,
in this case the program did get executed (notice the System.out.println
"main() execution started" message that got printed) and failed at runtime.

Would it be possible to make these two behaviours consistent and detect
such cases and abort early here too? Or would that add too much
complexity to this feature?

Finally, any thoughts on the error messages for this feature to make it
a bit easier in terms of debugging (classloading) issues like these?

[1] http://mail.openjdk.java.net/pipermail/jdk-dev/2018-June/001425.html
[2] http://mail.openjdk.java.net/pipermail/jdk-dev/2018-June/001438.html

-Jaikiran

Re: Source file launcher - Handling of pre-compiled classes of the source file

[Jaikiran Pai]

Hello Peter,


On 14/09/18 1:03 PM, Peter Levart wrote:
>
> The check for main class is performed after compilation (which
> actually produces the main class name).
>
> I think it would be possible to check for all classes compiled from
> the source file after compilation without to much complication. The
> compilation produces classes and stores them into a Map byte[]>. The keySet() of that map is a Set of compiled class names.
> Each of them could be tested via .getResource() invoked upon the
> application class loader. The error could even point to the URL of the
> conflicting class file that way...
>

Thank you for moving this to the correct mailing list. Thanks for those
details about how it's done and what can be done to improve this. Should
I report this as an enhancement request at bugs.java.com? I don't have
access to the OpenJDK JIRA for creating this request there.

-Jaikiran

Re: [PATCH] JDK-7033681 - Improve the documentation of Arrays.asList

[Jaikiran Pai]

edOperationException} for
>> operations that aren't permitted. Certain implementations of the
>> returned
>> list might choose to throw the exception only if the call to such
>> methods
>> results in an actual change, whereas certain other implementations
>> may always
>> throw the exception when such methods are called.
> This UOE statement now redundant with the above.
>
This has now been removed in favour of the suggested change.

>
> I do think it's reasonable to have examples here. Interestingly, the
> original (pre-varargs) purpose of Arrays.asList() was to wrap an
> existing array in a list. With varargs, this method is probably now
> more often used to create a list from arguments. Both uses are valid,
> but the first is now less obvious and so I think deserves a new
> example. The latter remains valid even with List.of() and friends,
> since those methods make a copy of the array that might not be
> necessary in all cases.
>
> Here's what I'd suggest:
>
>  - add a simple (~2 line) example showing wrapping of an existing array
>    in a list
Done. Added a new example to show this:

 * This method provides a way to wrap an existing array:
 * 
 * Integer[] numbers = ...
 * ...
 * List<Integer> values = Arrays.asList(numbers);
 * 

While adding this, I realized that the current javadoc doesn't mention
anything about the behaviour of this method when a null array is passed
to it. The implementation currently throws a NullPointerException. So I
went ahead and updated the javadoc to include:

 * @throws NullPointerException if the passed array is {@code null}


>
>  - restore the "Three Stooges" example (beginning with "This method
> also provides...")

Done.
>
>  - add a note emphasizing that the returned list is NOT unmodifiable.
> it's a common mistake to assume that it is. I'd also put in a
> reference to List.html#unmodifiable here in case people do want an
> unmodifiable list.

Done. I looked up the javadoc style guide[1] (is that still relevant,
especially for contributions to the JDK itself?) and javadoc of some
"recent" files within the JDK code, to see what's the current way to
emphasize certain parts of the comments. I couldn't find anything
conclusive, so I decided to use "". Let me know if there's some
other preferred way.
>
>>  * @param  the class of the objects in the array
>>  * @param a the array by which the list will be backed
>>  * @return a list view of the specified array
>>  * @see List#of()
>
> OK. We probably don't need the @see for List.of if there's a reference
> to the "unmodifiable lists" section above.

Done. "@see List#of()" is now removed in this updated version. Bernd,
let us know if you agree that this update to include reference to the
List.html#unmodifiable covers the use case for which we introduced the
List#of() reference.
>
>
> I can sponsor this change for you. Since we're changing testable
> assertions, this will also require a CSR request. I'll take care of
> that for you too.
Thank you :)

[1]
http://www.oracle.com/technetwork/java/javase/documentation/index-137868.html#styleguide

-Jaikiran

# HG changeset patch
# User Jaikiran Pai 
# Date 1535208403 -19800
#  Sat Aug 25 20:16:43 2018 +0530
# Node ID fbb71a7edc1aeeb6065e309a3efa67bc7ead6a3c
# Parent  6c394ed56b07d453226e064bc011528cd89b7d69
JDK-7033681 Improve the javadoc of Arrays#toList()

diff --git a/src/java.base/share/classes/java/util/Arrays.java 
b/src/java.base/share/classes/java/util/Arrays.java
--- a/src/java.base/share/classes/java/util/Arrays.java
+++ b/src/java.base/share/classes/java/util/Arrays.java
@@ -28,6 +28,7 @@
 import jdk.internal.HotSpotIntrinsicCandidate;
 import jdk.internal.util.ArraysSupport;
 
+import java.io.Serializable;
 import java.lang.reflect.Array;
 import java.util.concurrent.ForkJoinPool;
 import java.util.function.BinaryOperator;
@@ -4288,11 +4289,25 @@
 // Misc
 
 /**
- * Returns a fixed-size list backed by the specified array.  (Changes to
- * the returned list "write through" to the array.)  This method acts
- * as bridge between array-based and collection-based APIs, in
- * combination with {@link Collection#toArray}.  The returned list is
- * serializable and implements {@link RandomAccess}.
+ * Returns a fixed-size list backed by the specified array. Changes made to
+ * the array will be visible in the returned list, and changes made to the
+ * list will be visible in the array. The returned list is
+ * {@link Serializable} and implements {@link RandomAccess}.
+ *
+ * The returned list can be changed only in certain ways. Operations
+ * that woul

Re: Source file launcher - Handling of pre-compiled classes of the source file

[Jaikiran Pai]

Thank you Jon.

-Jaikiran


On 15/09/18 10:34 PM, Jonathan Gibbons wrote:
> Jaikiran,
>
> This issue is on our radar; there is no need for any additional action
> on your part at this point.
>
> -- Jon
>
>
> On 9/14/18 7:50 PM, Jaikiran Pai wrote:
>> Hello Peter,
>>
>>
>> On 14/09/18 1:03 PM, Peter Levart wrote:
>>> The check for main class is performed after compilation (which
>>> actually produces the main class name).
>>>
>>> I think it would be possible to check for all classes compiled from
>>> the source file after compilation without to much complication. The
>>> compilation produces classes and stores them into a Map>> byte[]>. The keySet() of that map is a Set of compiled class names.
>>> Each of them could be tested via .getResource() invoked upon the
>>> application class loader. The error could even point to the URL of the
>>> conflicting class file that way...
>>>
>> Thank you for moving this to the correct mailing list. Thanks for those
>> details about how it's done and what can be done to improve this. Should
>> I report this as an enhancement request at bugs.java.com? I don't have
>> access to the OpenJDK JIRA for creating this request there.
>>
>> -Jaikiran
>

Re: [PATCH] JDK-7033681 - Improve the documentation of Arrays.asList

[Jaikiran Pai]

Hello Stuart,


On 19/09/18 11:06 PM, Stuart Marks wrote:
>  
>
>> While adding this, I realized that the current javadoc doesn't mention
>> anything about the behaviour of this method when a null array is passed
>> to it. The implementation currently throws a NullPointerException. So I
>> went ahead and updated the javadoc to include:
>>
>>  * @throws NullPointerException if the passed array is {@code null}
>
> Strictly speaking this isn't necessary, as the class doc has the
> following statement:
>
>>  * The methods in this class all throw a {@code
>> NullPointerException},
>>  * if the specified array reference is null, except where noted.
>

I hadn't noticed that :)
>
>
> **
>
> The patch you sent is pretty good as it stands, but there were a
> couple minor things that I think could still be improved. In the
> interest of time, instead of asking you to make the changes, I went
> ahead and modified the patch myself. The modifications are as follows:
>
> - * The returned list can be changed only in certain ways.
> Operations
> - * that would change the size of the returned list leave the list
> unchanged
> - * and throw {@link UnsupportedOperationException}.
>
> The first sentence didn't seem right to me, as it's quite vague. After
> some thought it finally occurred to me that what's necessary is a
> statement about the optional methods of the Collection interface. I've
> edited this paragraph thus:
>
> + * The returned list implements the optional Collection
> methods, except those
> + * that would change the size of the returned list. Those methods
> leave
> + * the list unchanged and throw {@link
> UnsupportedOperationException}.

This looks fine to me.
>
> In the last paragraph I inverted the sense of "not unmodifiable" and
> added a link to the Collections.unmodifiableList method.
>
> + * The list returned by this method is modifiable.
> + * To create an unmodifiable list, see
> + * {@link Collections#unmodifiableList Collections.unmodifiableList}
> + * or Unmodifiable Lists.
This change too looks good.

>
>
> I've changed the  code samples to use {@code
> ...}. This allows use of < and > instead of HTML entities <
> and >.
Thank you, I'll keep that in mind for any future changes like this.
>
> Finally, I had to change the changeset metadata to conform to the
> OpenJDK rules. Specifically, the changeset author is required to be an
> OpenJDK user name. Since you don't have one, I've listed your email
> address in the Contributed-by line of the changeset comment.
Thank you. More of a FYI and if it matters from a process point of view
- in a couple of my earlier contributions, the sponsors have used the
"Contributed-by" line to be "Jaikiran Pai " like
here http://hg.openjdk.java.net/jdk/jdk/rev/6c394ed56b07. I don't have
any specific preference on which one is used.

>
> I've attached the modified patch. If you're ok with it, I'll proceed
> with filing a CSR.
I re-read the whole javadoc contained in your modified patch and it all
looks good to me. Thank you very much for the help in sponsoring,
reviewing and providing detailed inputs during the review.

-Jaikiran

Re: [PATCH] JDK-7033681 - Improve the documentation of Arrays.asList

[Jaikiran Pai]

On 21/09/18 1:04 AM, Stuart Marks wrote:
>
>
> Since you mentioned it I'll proceed with "Jaikiran Pai
> " since that looks a bit more "official". 

Sounds fine to me and yes it indeed is a bit more official one. Thank you.

-Jaikiran

Re: [PATCH] JDK-7033681 - Improve the documentation of Arrays.asList

[Jaikiran Pai]

Hello Stuart,

Thank you very much for your help in sponsoring this as well as the
detailed reviews and suggestions.

-Jaikiran



On Thursday, September 27, 2018, Stuart Marks 
wrote:

> Hi, I just wanted to mention that I pushed the changeset for this bug:
>
> http://hg.openjdk.java.net/jdk/jdk/rev/2ee7e1b7ba66
>
> Thanks for your contribution to OpenJDK!
>
> s'marks
>

JarFile constructor throws undocumented exception (only) on Windows OS

[Jaikiran Pai]

Please consider the following trivial code:

import java.util.jar.*;
import java.io.*;

public class JarFileTest {
    public static void main(final String[] args) throws Exception {
        final String tmpDir = System.getProperty("java.io.tmpdir");
        try {
            final JarFile jarFile = new JarFile(tmpDir + File.separator
+ "*");
        } catch (IOException ioe) {
            System.out.println("Got the expected IOException " + ioe);
        }
    }
}

The constructor of the JarFile is passed a string which intentionally is
an (wildcard) invalid path. The above code (as expected) throws an
IOException on *nix systems across various version of Java (tested
against Java 8, 11). The same code throws an IOException (as expected)
in Java 8 on Windows OS too. However, in Java 11, on Windows OS, this
code throws a different exception (java.nio.file.InvalidPathException)
which isn't IOException or a subclass of it:

Exception in thread "main" java.nio.file.InvalidPathException: Illegal
char <*> at index 38: C:\Users\someone\AppData\Local\Temp\1\*
at
java.base/sun.nio.fs.WindowsPathParser.normalize(WindowsPathParser.java:182)
at java.base/sun.nio.fs.WindowsPathParser.parse(WindowsPathParser.java:153)
at java.base/sun.nio.fs.WindowsPathParser.parse(WindowsPathParser.java:77)
at java.base/sun.nio.fs.WindowsPath.parse(WindowsPath.java:92)
at
java.base/sun.nio.fs.WindowsFileSystem.getPath(WindowsFileSystem.java:229)
at java.base/java.io.File.toPath(File.java:2290)
at java.base/java.util.zip.ZipFile$Source.get(ZipFile.java:1220)
at
java.base/java.util.zip.ZipFile$CleanableResource.(ZipFile.java:727)
at java.base/java.util.zip.ZipFile$CleanableResource.get(ZipFile.java:845)
at java.base/java.util.zip.ZipFile.(ZipFile.java:245)
at java.base/java.util.zip.ZipFile.(ZipFile.java:175)
at java.base/java.util.jar.JarFile.(JarFile.java:341)
at java.base/java.util.jar.JarFile.(JarFile.java:312)
at java.base/java.util.jar.JarFile.(JarFile.java:251)
at JarFileTest.main(JarFileTest.java:8)


The javadoc of JarFile constructor(s) mentions that:

 * @throws IOException if an I/O error has occurred

Given that the javadoc doesn't mention anything about this other
exception, would this throwing of java.nio.file.InvalidPathException be
considered a bug in the implementation?

If this indeed is considered a bug, it appears to be the code in
java.util.zip.ZipFile$Source.get method which calls File#toPath(), which
as per its javadoc is indeed expected to throw an
java.nio.file.InvalidPathException if a Path cannot be constructed out
of the File. Looking at the implementation of the underlying
java.nio.file.FileSystem on *nix and Windows, they differ when it comes
to whether or not the exception gets thrown (Unix one seems to always
return a result). But keeping the implementation of
java.nio.file.FileSystem aside, I guess the
java.util.zip.ZipFile$Source.get code needs to have a catch block for
the InvalidPathException to wrap that into a IOException? Something like:


# HG changeset patch
# User Jaikiran Pai 
# Date 1538645309 -19800
#  Thu Oct 04 14:58:29 2018 +0530
# Node ID ff1bfd8cc9a3b385716fa5191bb68989d552f87e
# Parent  8705c6d536c5c197d0210acccf145ebc48f90227
Wrap and throw an IOException when a InvalidPathException is thrown

diff --git a/src/java.base/share/classes/java/util/zip/ZipFile.java
b/src/java.base/share/classes/java/util/zip/ZipFile.java
--- a/src/java.base/share/classes/java/util/zip/ZipFile.java
+++ b/src/java.base/share/classes/java/util/zip/ZipFile.java
@@ -35,6 +35,7 @@
 import java.lang.ref.Cleaner.Cleanable;
 import java.nio.charset.Charset;
 import java.nio.charset.StandardCharsets;
+import java.nio.file.InvalidPathException;
 import java.nio.file.attribute.BasicFileAttributes;
 import java.nio.file.Files;
 import java.util.ArrayDeque;
@@ -1218,8 +1219,13 @@
 
 
 static Source get(File file, boolean toDelete) throws IOException {
-    Key key = new Key(file,
-  Files.readAttributes(file.toPath(),
BasicFileAttributes.class));
+    final Key key;
+    try {
+    key = new Key(file,
+    Files.readAttributes(file.toPath(),
BasicFileAttributes.class));
+    } catch (InvalidPathException ipe) {
+    throw new IOException(ipe);
+    }
 Source src;
 synchronized (files) {
 src = files.get(key);


Any thoughts?


-Jaikiran

Re: JarFile constructor throws undocumented exception (only) on Windows OS

[Jaikiran Pai]

Just for reference - this came up while debugging an issue, that one of
our users of Apache Ant project raised here
https://bz.apache.org/bugzilla/show_bug.cgi?id=62764#c8

-Jaikiran


On 04/10/18 3:06 PM, Jaikiran Pai wrote:
> Please consider the following trivial code:
>
> import java.util.jar.*;
> import java.io.*;
>
> public class JarFileTest {
>     public static void main(final String[] args) throws Exception {
>         final String tmpDir = System.getProperty("java.io.tmpdir");
>         try {
>             final JarFile jarFile = new JarFile(tmpDir + File.separator
> + "*");
>         } catch (IOException ioe) {
>             System.out.println("Got the expected IOException " + ioe);
>         }
>     }
> }
>
> The constructor of the JarFile is passed a string which intentionally is
> an (wildcard) invalid path. The above code (as expected) throws an
> IOException on *nix systems across various version of Java (tested
> against Java 8, 11). The same code throws an IOException (as expected)
> in Java 8 on Windows OS too. However, in Java 11, on Windows OS, this
> code throws a different exception (java.nio.file.InvalidPathException)
> which isn't IOException or a subclass of it:
>
> Exception in thread "main" java.nio.file.InvalidPathException: Illegal
> char <*> at index 38: C:\Users\someone\AppData\Local\Temp\1\*
> at
> java.base/sun.nio.fs.WindowsPathParser.normalize(WindowsPathParser.java:182)
> at java.base/sun.nio.fs.WindowsPathParser.parse(WindowsPathParser.java:153)
> at java.base/sun.nio.fs.WindowsPathParser.parse(WindowsPathParser.java:77)
> at java.base/sun.nio.fs.WindowsPath.parse(WindowsPath.java:92)
> at
> java.base/sun.nio.fs.WindowsFileSystem.getPath(WindowsFileSystem.java:229)
> at java.base/java.io.File.toPath(File.java:2290)
> at java.base/java.util.zip.ZipFile$Source.get(ZipFile.java:1220)
> at
> java.base/java.util.zip.ZipFile$CleanableResource.(ZipFile.java:727)
> at java.base/java.util.zip.ZipFile$CleanableResource.get(ZipFile.java:845)
> at java.base/java.util.zip.ZipFile.(ZipFile.java:245)
> at java.base/java.util.zip.ZipFile.(ZipFile.java:175)
> at java.base/java.util.jar.JarFile.(JarFile.java:341)
> at java.base/java.util.jar.JarFile.(JarFile.java:312)
> at java.base/java.util.jar.JarFile.(JarFile.java:251)
> at JarFileTest.main(JarFileTest.java:8)
>
>
> The javadoc of JarFile constructor(s) mentions that:
>
>  * @throws IOException if an I/O error has occurred
>
> Given that the javadoc doesn't mention anything about this other
> exception, would this throwing of java.nio.file.InvalidPathException be
> considered a bug in the implementation?
>
> If this indeed is considered a bug, it appears to be the code in
> java.util.zip.ZipFile$Source.get method which calls File#toPath(), which
> as per its javadoc is indeed expected to throw an
> java.nio.file.InvalidPathException if a Path cannot be constructed out
> of the File. Looking at the implementation of the underlying
> java.nio.file.FileSystem on *nix and Windows, they differ when it comes
> to whether or not the exception gets thrown (Unix one seems to always
> return a result). But keeping the implementation of
> java.nio.file.FileSystem aside, I guess the
> java.util.zip.ZipFile$Source.get code needs to have a catch block for
> the InvalidPathException to wrap that into a IOException? Something like:
>
>
> # HG changeset patch
> # User Jaikiran Pai 
> # Date 1538645309 -19800
> #  Thu Oct 04 14:58:29 2018 +0530
> # Node ID ff1bfd8cc9a3b385716fa5191bb68989d552f87e
> # Parent  8705c6d536c5c197d0210acccf145ebc48f90227
> Wrap and throw an IOException when a InvalidPathException is thrown
>
> diff --git a/src/java.base/share/classes/java/util/zip/ZipFile.java
> b/src/java.base/share/classes/java/util/zip/ZipFile.java
> --- a/src/java.base/share/classes/java/util/zip/ZipFile.java
> +++ b/src/java.base/share/classes/java/util/zip/ZipFile.java
> @@ -35,6 +35,7 @@
>  import java.lang.ref.Cleaner.Cleanable;
>  import java.nio.charset.Charset;
>  import java.nio.charset.StandardCharsets;
> +import java.nio.file.InvalidPathException;
>  import java.nio.file.attribute.BasicFileAttributes;
>  import java.nio.file.Files;
>  import java.util.ArrayDeque;
> @@ -1218,8 +1219,13 @@
>  
>  
>  static Source get(File file, boolean toDelete) throws IOException {
> -    Key key = new Key(file,
> -  Files.readAttributes(file.toPath(),
> BasicFileAttributes.class));
> +    final Key key;
> +    try {
> +    key = new Key(file,
> +    Files.readAttributes(file.toPath(),
> BasicFileAttributes.class));
> +    } catch (InvalidPathException ipe) {
> +    throw new IOException(ipe);
> +    }
>  Source src;
>  synchronized (files) {
>  src = files.get(key);
>
>
> Any thoughts?
>
>
> -Jaikiran
>

Re: JarFile constructor throws undocumented exception (only) on Windows OS

[Jaikiran Pai]

Hello Bernd,

On 05/10/18 2:51 PM, Bernd Eckenfels wrote:
> Why does that actually throw an Exception on Linux/Unix, it is a
> perfectly legal file name.
In the context of my test and the issue reported in Apache Ant, it fails
on Linux/Unix because there isn't actually a file present at that path.
So it's a genuine NoSuchFileException and _isn't_ an implementation bug:

Exception in thread "main" java.nio.file.NoSuchFileException: /tmp/*

The reason I used a non-existent file in my sample, is to illustrate the
difference in the exception thrown by Java 11 on Windows and *nix.

-Jaikiran


>
> Gruss
> Bernd
>
> Gruss
> Bernd
> -- 
> http://bernd.eckenfels.net
>  
> 
> *Von:* -2122936656m Auftrag von
> *Gesendet:* Freitag, Oktober 5, 2018 11:00 AM
> *An:* core-libs-dev@openjdk.java.net
> *Betreff:* Re: JarFile constructor throws undocumented exception
> (only) on Windows OS
>  
> Just for reference - this came up while debugging an issue, that one of
> our users of Apache Ant project raised here
> https://bz.apache.org/bugzilla/show_bug.cgi?id=62764#c8
>
> -Jaikiran
>
>
> On 04/10/18 3:06 PM, Jaikiran Pai wrote:
> > Please consider the following trivial code:
> >
> > import java.util.jar.*;
> > import java.io.*;
> >
> > public class JarFileTest {
> >     public static void main(final String[] args) throws Exception {
> >         final String tmpDir = System.getProperty("java.io.tmpdir");
> >         try {
> >             final JarFile jarFile = new JarFile(tmpDir + File.separator
> > + "*");
> >         } catch (IOException ioe) {
> >             System.out.println("Got the expected IOException " + ioe);
> >         }
> >     }
> > }
> >
> > The constructor of the JarFile is passed a string which
> intentionally is
> > an (wildcard) invalid path. The above code (as expected) throws an
> > IOException on *nix systems across various version of Java (tested
> > against Java 8, 11). The same code throws an IOException (as expected)
> > in Java 8 on Windows OS too. However, in Java 11, on Windows OS, this
> > code throws a different exception (java.nio.file.InvalidPathException)
> > which isn't IOException or a subclass of it:
> >
> > Exception in thread "main" java.nio.file.InvalidPathException: Illegal
> > char <*> at index 38: C:\Users\someone\AppData\Local\Temp\1\*
> > at
> >
> java.base/sun.nio.fs.WindowsPathParser.normalize(WindowsPathParser.java:182)
>
> > at
> java.base/sun.nio.fs.WindowsPathParser.parse(WindowsPathParser.java:153)
> > at
> java.base/sun.nio.fs.WindowsPathParser.parse(WindowsPathParser.java:77)
> > at java.base/sun.nio.fs.WindowsPath.parse(WindowsPath.java:92)
> > at
> >
> java.base/sun.nio.fs.WindowsFileSystem.getPath(WindowsFileSystem.java:229)
>
> > at java.base/java.io.File.toPath(File.java:2290)
> > at java.base/java.util.zip.ZipFile$Source.get(ZipFile.java:1220)
> > at
> >
> java.base/java.util.zip.ZipFile$CleanableResource.(ZipFile.java:727)
>
> > at
> java.base/java.util.zip.ZipFile$CleanableResource.get(ZipFile.java:845)
> > at java.base/java.util.zip.ZipFile.(ZipFile.java:245)
> > at java.base/java.util.zip.ZipFile.(ZipFile.java:175)
> > at java.base/java.util.jar.JarFile.(JarFile.java:341)
> > at java.base/java.util.jar.JarFile.(JarFile.java:312)
> > at java.base/java.util.jar.JarFile.(JarFile.java:251)
> > at JarFileTest.main(JarFileTest.java:8)
> >
> >
> > The javadoc of JarFile constructor(s) mentions that:
> >
> >  * @throws IOException if an I/O error has occurred
> >
> > Given that the javadoc doesn't mention anything about this other
> > exception, would this throwing of java.nio.file.InvalidPathException be
> > considered a bug in the implementation?
> >
> > If this indeed is considered a bug, it appears to be the code in
> > java.util.zip.ZipFile$Source.get method which calls File#toPath(),
> which
> > as per its javadoc is indeed expected to throw an
> > java.nio.file.InvalidPathException if a Path cannot be constructed out
> > of the File. Looking at the implementation of the underlying
> > java.nio.file.FileSystem on *nix and Windows, they differ when it comes
> > to whether or not the exception gets thrown (Unix one seems to always
> > return a result). But keeping the implementation of
> > java.nio.file.FileSystem aside, I guess the
> > java.util.zip.ZipFile$Source.get code needs to have a catch block for
> > 

Re: JarFile constructor throws undocumented exception (only) on Windows OS

[Jaikiran Pai]

Hello Alan,


On 05/10/18 4:15 PM, Alan Bateman wrote:
> On 04/10/2018 10:36, Jaikiran Pai wrote:
>> :
>>
>>
>> The javadoc of JarFile constructor(s) mentions that:
>>
>>   * @throws IOException if an I/O error has occurred
>>
>> Given that the javadoc doesn't mention anything about this other
>> exception, would this throwing of java.nio.file.InvalidPathException be
>> considered a bug in the implementation?
>>
> Yes, it's a bug in the ZipFile code as it's not specified to throw
> this unchecked exception. Note that the issue is not specific to
> Windows, you'll see the same thing on Linux/other with other garbage
> input - a good example to try is a path string with NUL characters.

You are right indeed. Changing that sample code to something like:

final JarFile jarFile = new JarFile(tmpDir + File.separator + "abc\0xyz");

does indeed end up throwing a java.nio.file.InvalidPathException even on
*nix, on Java 11.

I don't have access to create an issue for this in OpenJDK JIRA, so I'll
go ahead and create one at bugs.java.com.

-Jaikiran

Re: JarFile constructor throws undocumented exception (only) on Windows OS

[Jaikiran Pai]

> I don't have access to create an issue for this in OpenJDK JIRA, so I'll
> go ahead and create one at bugs.java.com.
>
I just submitted the report. Internal review id 9057522 has been
assigned to the issue.

-Jaikiran

[PATCH] JDK-8211765 - JarFile constructor throws undocumented exception

[Jaikiran Pai]

Thank you Chris.

I've attached a patch here which catches the InvalidPathException within
the implementation of ZipFile and wraps and throws a IOException. It's
the same patch which I mentioned earlier in this thread, but now also
includes jtreg testcase to reproduce the issue and verify the fix. I
have run the jtreg test with and without this patch on my *nix system
and it behaves as expected (fails without the fix and passes with it). I
don't have a Windows OS at hand to test this patch and the testcase on it.

I also did a very basic check and went through the JarFile/ZipFile
constructor code path to see if there are other places in that flow
which might need similar treatment. My not-so-thorough check didn't show
up any other such instance.

Could I please get help from someone who can sponsor this patch and help
with the review too?

-Jaikiran


On 05/10/18 6:21 PM, Chris Hegarty wrote:
>> On 5 Oct 2018, at 12:08, Jaikiran Pai  wrote:
>>
>>> I don't have access to create an issue for this in OpenJDK JIRA, so I'll
>>> go ahead and create one at bugs.java.com.
>>>
>> I just submitted the report. Internal review id 9057522 has been
>> assigned to the issue.
> And now in the JDK project:
>
>   https://bugs.openjdk.java.net/browse/JDK-8211765
>
> -Chris.

# HG changeset patch
# User Jaikiran Pai 
# Date 1538645309 -19800
#  Thu Oct 04 14:58:29 2018 +0530
# Node ID 957376a6e53c751fa144d0e41e063ef9179a9275
# Parent  e75f6076d391a6081e621d64641526c99bf8c5b2
JDK-8211765 Wrap and throw an IOException when a InvalidPathException is thrown 
from within ZipFile constructor

diff --git a/src/java.base/share/classes/java/util/zip/ZipFile.java 
b/src/java.base/share/classes/java/util/zip/ZipFile.java
--- a/src/java.base/share/classes/java/util/zip/ZipFile.java
+++ b/src/java.base/share/classes/java/util/zip/ZipFile.java
@@ -35,6 +35,7 @@
 import java.lang.ref.Cleaner.Cleanable;
 import java.nio.charset.Charset;
 import java.nio.charset.StandardCharsets;
+import java.nio.file.InvalidPathException;
 import java.nio.file.attribute.BasicFileAttributes;
 import java.nio.file.Files;
 import java.util.ArrayDeque;
@@ -1218,8 +1219,13 @@
 
 
 static Source get(File file, boolean toDelete) throws IOException {
-Key key = new Key(file,
-  Files.readAttributes(file.toPath(), 
BasicFileAttributes.class));
+final Key key;
+try {
+key = new Key(file,
+Files.readAttributes(file.toPath(), 
BasicFileAttributes.class));
+} catch (InvalidPathException ipe) {
+throw new IOException(ipe);
+}
 Source src;
 synchronized (files) {
 src = files.get(key);
diff --git a/test/jdk/java/util/jar/JarFile/Constructor.java 
b/test/jdk/java/util/jar/JarFile/Constructor.java
--- a/test/jdk/java/util/jar/JarFile/Constructor.java
+++ b/test/jdk/java/util/jar/JarFile/Constructor.java
@@ -23,7 +23,7 @@
 
 /**
  * @test
- * @bug 4842702
+ * @bug 4842702 8211765
  * @summary Check that constructors throw specified exceptions
  * @author Martin Buchholz
  */
@@ -63,5 +63,23 @@
 
 try { Unreached (new JarFile (new File ("NoSuchJar.jar"))); }
 catch (IOException e) {}
+
+// operating system specific
+final String osname = System.getProperty("os.name");
+if (osname.startsWith("Windows")) {
+doWindowsTests();
+} else {
+doUnixTests();
+}
+}
+
+private static void doWindowsTests() throws Exception {
+try { Unreached (new JarFile ("C:\\*")); } // invalid character
+catch (IOException e) {}
+}
+
+private static void doUnixTests() throws Exception {
+try { Unreached (new JarFile ("foo\ubar")); } // invalid character
+catch (IOException e) {}
 }
 }

Re: [PATCH] JDK-8211765 - JarFile constructor throws undocumented exception

[Jaikiran Pai]

Thank you Sherman.

-Jaikiran


On 05/10/18 9:59 PM, Xueming Shen wrote:
> Hi Jaikiran,
>
> "wrap to throw a IOE" appears good. I will sponsor this patch.
>
> Thanks!
> -Sherman
>
> On 10/5/18, 8:31 AM, Jaikiran Pai wrote:
>> Thank you Chris.
>>
>> I've attached a patch here which catches the InvalidPathException within
>> the implementation of ZipFile and wraps and throws a IOException. It's
>> the same patch which I mentioned earlier in this thread, but now also
>> includes jtreg testcase to reproduce the issue and verify the fix. I
>> have run the jtreg test with and without this patch on my *nix system
>> and it behaves as expected (fails without the fix and passes with it). I
>> don't have a Windows OS at hand to test this patch and the testcase
>> on it.
>>
>> I also did a very basic check and went through the JarFile/ZipFile
>> constructor code path to see if there are other places in that flow
>> which might need similar treatment. My not-so-thorough check didn't show
>> up any other such instance.
>>
>> Could I please get help from someone who can sponsor this patch and help
>> with the review too?
>>
>> -Jaikiran
>>
>>
>> On 05/10/18 6:21 PM, Chris Hegarty wrote:
>>>> On 5 Oct 2018, at 12:08, Jaikiran Pai 
>>>> wrote:
>>>>
>>>>> I don't have access to create an issue for this in OpenJDK JIRA,
>>>>> so I'll
>>>>> go ahead and create one at bugs.java.com.
>>>>>
>>>> I just submitted the report. Internal review id 9057522 has been
>>>> assigned to the issue.
>>> And now in the JDK project:
>>>
>>>    https://bugs.openjdk.java.net/browse/JDK-8211765
>>>
>>> -Chris.
>

Filesystem case sensitive check and java.io.File#equals

[Jaikiran Pai]

In one of the projects that I'm involved in, we do a bunch of file
operations which sometimes need to check if a particular filesystem is
case sensitive. I was thinking of using java.io.File#equals since its
javadoc states:


/**
 * Tests this abstract pathname for equality with the given object.
 * Returns true if and only if the argument is not
 * null and is an abstract pathname that denotes the
same file
 * or directory as this abstract pathname.  Whether or not two abstract
 * pathnames are equal depends upon the underlying *system*.  On UNIX
 * systems, alphabetic case is significant in comparing pathnames;
on Microsoft Windows
 * systems it is not.
 *
 * @param   obj   The object to be compared with this abstract pathname
 *
 * @return  true if and only if the objects are the same;
 *  false otherwise
 */


My impression, based on that javadoc, was that the implementation of
that API will use the underlying _filesystem_ to decide whether or not
its case sensitive. However, my experiments on a macOS which is case
insensitive and also a basic check of the implementation code in the
JDK, shows that this uses a lexicographic check on the string
representation of the paths. Is that what this javadoc means by
"underlying system". I understand that there's a further sentence in
that javadoc which says UNIX systems are case significant and Windows
isn't. However, it wasn't fully clear to me that this API wouldn't
delegate it to the underlying filesystem on the OS.

While we are at it, is there a better API that can be used to do such a
case sensitivity check of the underlying filesystem? I checked the APIs
in java.nio.file but couldn't find anything relevant. If there isn't any
such API currently, is that something that can be introduced?

-Jaikiran

Re: Filesystem case sensitive check and java.io.File#equals

[Jaikiran Pai]

Hi Alan,

On 07/11/18 7:15 PM, Alan Bateman wrote:
> On 07/11/2018 13:13, Jaikiran Pai wrote:
>> :
>>
>>
>> My impression, based on that javadoc, was that the implementation of
>> that API will use the underlying _filesystem_ to decide whether or not
>> its case sensitive. However, my experiments on a macOS which is case
>> insensitive and also a basic check of the implementation code in the
>> JDK, shows that this uses a lexicographic check on the string
>> representation of the paths. Is that what this javadoc means by
>> "underlying system". I understand that there's a further sentence in
>> that javadoc which says UNIX systems are case significant and Windows
>> isn't. However, it wasn't fully clear to me that this API wouldn't
>> delegate it to the underlying filesystem on the OS.
> Maybe the javadoc could be clearer but having an equals method
> delegate to the underlying file system would be very problematic
> (think URL equals or cases where the file path is relative or doesn't
> locate a file).

I see what you mean. About the javadoc, do you think it is worth
improving and if so, should I file an enhancement at bugs.java.com?

>
>> While we are at it, is there a better API that can be used to do such a
>> case sensitivity check of the underlying filesystem? I checked the APIs
>> in java.nio.file but couldn't find anything relevant. If there isn't any
>> such API currently, is that something that can be introduced?
>>
> This has been looked into a couple of times over the years,
> suggestions have included FileStore exposing a comparator that uses
> the rules of a specific underlying file system or volume. This turns
> out to be very complicated as it means looking beyond case sensitively
> issues and into topics such as case preservation, Unicode
> normalization forms, per volume case mapping tables, and handling of
> limits and truncation.
So I guess, that leaves us (the client applications) to rely on some
kind of file creation tricks like the one below to figure this out:

        // create a temp file in a fresh directory
    final Path tmpDir = Files.createTempDirectory(null);
    final Path tmpFile = Files.createTempFile(tmpDir, null, null);
    tmpFile.toFile().deleteOnExit();
    tmpDir.toFile().deleteOnExit();
    // now check if a file with that same name but different case is
considered to exist
    final boolean existsAsLowerCase =
Files.exists(Paths.get(tmpDir.toString(),
tmpFile.getFileName().toString().toLowerCase()));
    final boolean existsAsUpperCase =
Files.exists(Paths.get(tmpDir.toString(),
tmpFile.getFileName().toString().toUpperCase()));
    // if the temp file that we created is found to not exist in a
particular "case", then
    // the filesystem is case sensitive
    final boolean filesystemCaseSensitive = existsAsLowerCase ==
false || existsAsUpperCase == false;

One final question - is there anywhere in the JDK code, where files are
auto created (for whatever purpose)? If such a construct exists, maybe
during the initialization of each FileStore implementation, it could do
such tricks (maybe in a much better and performant way) and figure out
this detail and then expose it some way? It feels hacky to do it in the
JDK, so I fully understand if such a construct won't be entertained.

-Jaikiran

Re: Filesystem case sensitive check and java.io.File#equals

[Jaikiran Pai]

Hello Roger,

That indeed is a much cleaner approach. Thank you for that example.

-Jaikiran


On 07/11/18 8:37 PM, Roger Riggs wrote:
> Hi Jaikiran,
>
> To check if two pathnames are the same file,
> java.nio.file.Files.isSameFile(path1, path2)
> is cleaner.  It uses the file system specific mechanisms to determine
> if the two paths
> refer to the identical file.  Traversing symbolic links, etc.
>
> Something like:
>             Path dir = ...;   // supply a directory to test a
> particular file system
>     Path path1 = Files.createTempFile(dir, "MixedCase", "");
>     Path path2 =
> Path.of(path1.toString().toLowerCase(Locale.US));
>     return Files.isSameFile(path1, path2);
>
> or similar...
>
> Roger
>
> On 11/07/2018 09:27 AM, Jaikiran Pai wrote:
>> Hi Alan,
>>
>> On 07/11/18 7:15 PM, Alan Bateman wrote:
>>> On 07/11/2018 13:13, Jaikiran Pai wrote:
>>>> :
>>>>
>>>>
>>>> My impression, based on that javadoc, was that the implementation of
>>>> that API will use the underlying _filesystem_ to decide whether or not
>>>> its case sensitive. However, my experiments on a macOS which is case
>>>> insensitive and also a basic check of the implementation code in the
>>>> JDK, shows that this uses a lexicographic check on the string
>>>> representation of the paths. Is that what this javadoc means by
>>>> "underlying system". I understand that there's a further sentence in
>>>> that javadoc which says UNIX systems are case significant and Windows
>>>> isn't. However, it wasn't fully clear to me that this API wouldn't
>>>> delegate it to the underlying filesystem on the OS.
>>> Maybe the javadoc could be clearer but having an equals method
>>> delegate to the underlying file system would be very problematic
>>> (think URL equals or cases where the file path is relative or doesn't
>>> locate a file).
>> I see what you mean. About the javadoc, do you think it is worth
>> improving and if so, should I file an enhancement at bugs.java.com?
>>
>>>> While we are at it, is there a better API that can be used to do
>>>> such a
>>>> case sensitivity check of the underlying filesystem? I checked the
>>>> APIs
>>>> in java.nio.file but couldn't find anything relevant. If there
>>>> isn't any
>>>> such API currently, is that something that can be introduced?
>>>>
>>> This has been looked into a couple of times over the years,
>>> suggestions have included FileStore exposing a comparator that uses
>>> the rules of a specific underlying file system or volume. This turns
>>> out to be very complicated as it means looking beyond case sensitively
>>> issues and into topics such as case preservation, Unicode
>>> normalization forms, per volume case mapping tables, and handling of
>>> limits and truncation.
>> So I guess, that leaves us (the client applications) to rely on some
>> kind of file creation tricks like the one below to figure this out:
>>
>>      // create a temp file in a fresh directory
>>  final Path tmpDir = Files.createTempDirectory(null);
>>  final Path tmpFile = Files.createTempFile(tmpDir, null, null);
>>  tmpFile.toFile().deleteOnExit();
>>  tmpDir.toFile().deleteOnExit();
>>  // now check if a file with that same name but different
>> case is
>> considered to exist
>>  final boolean existsAsLowerCase =
>> Files.exists(Paths.get(tmpDir.toString(),
>> tmpFile.getFileName().toString().toLowerCase()));
>>  final boolean existsAsUpperCase =
>> Files.exists(Paths.get(tmpDir.toString(),
>> tmpFile.getFileName().toString().toUpperCase()));
>>  // if the temp file that we created is found to not exist in a
>> particular "case", then
>>  // the filesystem is case sensitive
>>  final boolean filesystemCaseSensitive = existsAsLowerCase ==
>> false || existsAsUpperCase == false;
>>
>> One final question - is there anywhere in the JDK code, where files are
>> auto created (for whatever purpose)? If such a construct exists, maybe
>> during the initialization of each FileStore implementation, it could do
>> such tricks (maybe in a much better and performant way) and figure out
>> this detail and then expose it some way? It feels hacky to do it in the
>> JDK, so I fully understand if such a construct won't be entertained.
>>
>> -Jaikiran
>>
>>
>>
>

Re: Filesystem case sensitive check and java.io.File#equals

[Jaikiran Pai]

A slightly related question - I used the example that Roger showed and
it mostly worked. However, Files.isSameFile throws a
java.nio.file.NoSuchFileException since the path2 doesn't exist (on a
case sensitive file system). I checked the javadoc of Files.isSameFile
and couldn't find a clear mention if this is expected. It does talk
about the existence checks that might be carried out in this
implementation, but doesn't mention that it will throw an exception in
the absence of either of the passed paths. Is this expected? Or should
the implementation internally catch this exception and return false?

For now, in my implementation, I have explicitly caught this
NoSuchFileException, around the Files.isSameFile statement and consider
it to imply the filesystem is case sensitive.

-Jaikiran


On 07/11/18 8:37 PM, Roger Riggs wrote:
> Hi Jaikiran,
>
> To check if two pathnames are the same file,
> java.nio.file.Files.isSameFile(path1, path2)
> is cleaner.  It uses the file system specific mechanisms to determine
> if the two paths
> refer to the identical file.  Traversing symbolic links, etc.
>
> Something like:
>             Path dir = ...;   // supply a directory to test a
> particular file system
>     Path path1 = Files.createTempFile(dir, "MixedCase", "");
>     Path path2 =
> Path.of(path1.toString().toLowerCase(Locale.US));
>     return Files.isSameFile(path1, path2);
>
> or similar...
>
> Roger
>
> On 11/07/2018 09:27 AM, Jaikiran Pai wrote:
>> Hi Alan,
>>
>> On 07/11/18 7:15 PM, Alan Bateman wrote:
>>> On 07/11/2018 13:13, Jaikiran Pai wrote:
>>>> :
>>>>
>>>>
>>>> My impression, based on that javadoc, was that the implementation of
>>>> that API will use the underlying _filesystem_ to decide whether or not
>>>> its case sensitive. However, my experiments on a macOS which is case
>>>> insensitive and also a basic check of the implementation code in the
>>>> JDK, shows that this uses a lexicographic check on the string
>>>> representation of the paths. Is that what this javadoc means by
>>>> "underlying system". I understand that there's a further sentence in
>>>> that javadoc which says UNIX systems are case significant and Windows
>>>> isn't. However, it wasn't fully clear to me that this API wouldn't
>>>> delegate it to the underlying filesystem on the OS.
>>> Maybe the javadoc could be clearer but having an equals method
>>> delegate to the underlying file system would be very problematic
>>> (think URL equals or cases where the file path is relative or doesn't
>>> locate a file).
>> I see what you mean. About the javadoc, do you think it is worth
>> improving and if so, should I file an enhancement at bugs.java.com?
>>
>>>> While we are at it, is there a better API that can be used to do
>>>> such a
>>>> case sensitivity check of the underlying filesystem? I checked the
>>>> APIs
>>>> in java.nio.file but couldn't find anything relevant. If there
>>>> isn't any
>>>> such API currently, is that something that can be introduced?
>>>>
>>> This has been looked into a couple of times over the years,
>>> suggestions have included FileStore exposing a comparator that uses
>>> the rules of a specific underlying file system or volume. This turns
>>> out to be very complicated as it means looking beyond case sensitively
>>> issues and into topics such as case preservation, Unicode
>>> normalization forms, per volume case mapping tables, and handling of
>>> limits and truncation.
>> So I guess, that leaves us (the client applications) to rely on some
>> kind of file creation tricks like the one below to figure this out:
>>
>>      // create a temp file in a fresh directory
>>  final Path tmpDir = Files.createTempDirectory(null);
>>  final Path tmpFile = Files.createTempFile(tmpDir, null, null);
>>  tmpFile.toFile().deleteOnExit();
>>  tmpDir.toFile().deleteOnExit();
>>  // now check if a file with that same name but different
>> case is
>> considered to exist
>>  final boolean existsAsLowerCase =
>> Files.exists(Paths.get(tmpDir.toString(),
>> tmpFile.getFileName().toString().toLowerCase()));
>>  final boolean existsAsUpperCase =
>> Files.exists(Paths.get(tmpDir.toString(),
>> tmpFile.getFileName().toString().toUpperCase()));
>>  // if the temp file that we created is found to no

Re: Filesystem case sensitive check and java.io.File#equals

[Jaikiran Pai]

Thanks for the clarification, Alan.

-Jaikiran

On Thursday, November 8, 2018, Alan Bateman  wrote:
> On 08/11/2018 12:59, Jaikiran Pai wrote:
>>
>> A slightly related question - I used the example that Roger showed and
>> it mostly worked. However, Files.isSameFile throws a
>> java.nio.file.NoSuchFileException since the path2 doesn't exist (on a
>> case sensitive file system). I checked the javadoc of Files.isSameFile
>> and couldn't find a clear mention if this is expected. It does talk
>> about the existence checks that might be carried out in this
>> implementation, but doesn't mention that it will throw an exception in
>> the absence of either of the passed paths. Is this expected? Or should
>> the implementation internally catch this exception and return false?
>>
> IOException is correct, the methods in this API aren't specify all the
possible sub-classes. In a few places you will see specific exceptions
specified as "optional specific exception" where it make sense. We could
potentially improve the spec for the cannot access or does not exist cases
but hasn't been an issue to date.
>
> -Alan
>

Re: jar --verify operation mode checking mrjar validity

[Jaikiran Pai]

(resending using the email id I'm subscribed by, in this list)

On 02/12/18 3:43 PM, Alan Bateman wrote:
> On 01/12/2018 08:45, Christian Stein wrote:
>> Hi,
>>
>> jar --create (and --update) perform type API validation checks when
>> used in
>> combination with --release option. This detects invalid "version
>> overlays"
>> at package time, where the API doesn't match a previous one.
>>
>> Having a jar --verify mode that performs the same checks consuming an
>> already existing jar file would be useful as most "3rd-party packaging
>> tools" don't perform those checks.
>>
>> A possible work-around could be to explode an existing jar and
>> re-create it
>> using jar --create...
>>
> I think it would be useful to create a list of the popular tools and
> plugins in the eco system that create or update JAR files and see what
> their current capabilities are. The addition of modules and MR JARs
> have extended the JAR format quite a bit in recent years and it's not
> clear if the eco system has caught up,

FWIW - We (Apache Ant) haven't yet caught up with many of these
changes/enhancements, both in what the Java tools (like jar, jmod
etc...) have introduced[1][2][3], nor in runtime aspects like
multi-release jars[4]. It's going to take us a while to catch up with
these current new enhancements.

[1] https://bz.apache.org/bugzilla/show_bug.cgi?id=62789
[2] https://www.mail-archive.com/user@ant.apache.org/msg42796.html
[3] https://www.mail-archive.com/dev@ant.apache.org/msg47740.html
[4] https://bz.apache.org/bugzilla/show_bug.cgi?id=62952

-Jaikiran

Re: jar --verify operation mode checking mrjar validity

[Jaikiran Pai]

On 02/12/18 3:43 PM, Alan Bateman wrote:
> On 01/12/2018 08:45, Christian Stein wrote:
>> Hi,
>>
>> jar --create (and --update) perform type API validation checks when
>> used in
>> combination with --release option. This detects invalid "version
>> overlays"
>> at package time, where the API doesn't match a previous one.
>>
>> Having a jar --verify mode that performs the same checks consuming an
>> already existing jar file would be useful as most "3rd-party packaging
>> tools" don't perform those checks.
>>
>> A possible work-around could be to explode an existing jar and
>> re-create it
>> using jar --create...
>>
> I think it would be useful to create a list of the popular tools and
> plugins in the eco system that create or update JAR files and see what
> their current capabilities are. The addition of modules and MR JARs
> have extended the JAR format quite a bit in recent years and it's not
> clear if the eco system has caught up,

FWIW - We (Apache Ant) haven't yet caught up with many of these
changes/enhancements, both in what the Java tools (like jar, jmod
etc...) have introduced[1][2][3], nor in runtime aspects like
multi-release jars[4]. It's going to take us a while to catch up with
these current new enhancements.

[1] https://bz.apache.org/bugzilla/show_bug.cgi?id=62789
[2] https://www.mail-archive.com/user@ant.apache.org/msg42796.html
[3] https://www.mail-archive.com/dev@ant.apache.org/msg47740.html
[4] https://bz.apache.org/bugzilla/show_bug.cgi?id=62952

-Jaikiran

Multi-release jar file - Should the "Multi-Release" mainfest attribute have a specific value?

[Jaikiran Pai]

Hi,

The Multi-Release JarFile support JEP[1] notes that the jar file is
expected to contain a main attribute:

Multi-Release: true

The JarFile javadoc[1] on the other hand makes no mention of the value
for such an attribute and just mentions that the Multi-Release main
attribute needs to be present:

"A multi-release jar file is a jar file that contains a manifest with a
main attribute named "Multi-Release",..."

Which one of these 2 docs is expected to be the formal specification?
I'm guessing it's the javadoc, in which case, is it intentional that the
value for this attribute isn't mentioned or is it just a miss? My quick
tests show that the implementation of this feature does indeed expect
the value to be a "true" for it to consider the versioned entries in a
multi-release jar.

[1] http://openjdk.java.net/jeps/238

[2]
https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/util/jar/JarFile.html

-Jaikiran

Re: Multi-release jar file - Should the "Multi-Release" mainfest attribute have a specific value?

[Jaikiran Pai]

Thank you Alan.

-Jaikiran

On 23/01/19 8:09 PM, Alan Bateman wrote:
> On 23/01/2019 14:27, Jaikiran Pai wrote:
>> Hi,
>>
>> The Multi-Release JarFile support JEP[1] notes that the jar file is
>> expected to contain a main attribute:
>>
>> Multi-Release: true
>>
>> The JarFile javadoc[1] on the other hand makes no mention of the value
>> for such an attribute and just mentions that the Multi-Release main
>> attribute needs to be present:
>>
>> "A multi-release jar file is a jar file that contains a manifest with a
>> main attribute named "Multi-Release",..."
>>
>> Which one of these 2 docs is expected to be the formal specification?
>> I'm guessing it's the javadoc, in which case, is it intentional that the
>> value for this attribute isn't mentioned or is it just a miss? My quick
>> tests show that the implementation of this feature does indeed expect
>> the value to be a "true" for it to consider the versioned entries in a
>> multi-release jar.
>>
> The JAR file spec [1] is the specification. There's a lengthy section
> defining MR JARs. The API docs are also specification, for the APIs
> that provide access to JAR files, including MR JARs. So yes, you need
> the "Multi-Release" attribute in the main section of the JAR file
> manifest and it needs to have a value of "true" to enable. There is
> also a JDK-specific property, documented in the JarFile docs, to
> override the configuration where needed.
>
> -Alan
>
> [1] https://docs.oracle.com/en/java/javase/11/docs/specs/jar/jar.html

Inputs on patch for JDK-8225763?

[Jaikiran Pai]

I am looking to contribute a patch for the enhancement noted in
https://bugs.openjdk.java.net/browse/JDK-8225763. The change itself
looks relatively straightforward to me and here's what I plan to do:

1. Have both java.util.zip.Inflater and java.util.zip.Deflater start
implementing the AutoCloseable interface

2. Have the close() method call the end() method

3. Add javadoc to the close() implementation to mention that the end()
method gets called

4. Add test(s) to verify that the close indeed actually "ends" the
inflater/deflater

5. This is more of a question - do we have to update the class level
javadoc of both these classes to make a mention that these classes have
started implementing the AutoCloseable starting version 14 (or whichever
version this change makes into) of Java?

Any other inputs/suggestions on what else might be needed in this change?

-Jaikiran

Re: Inputs on patch for JDK-8225763?

[Jaikiran Pai]

Hello Alan,

On 27/06/19 4:22 PM, Alan Bateman wrote:
> On 27/06/2019 11:16, Remi Forax wrote:
>> Is a boolean isEnded that records if end() was already called is not
>> enough ?
>>
> I'm sure Jaikiran will create tests that cover the different
> combinations of overriding end and close. It might enough for close to
> check if zsRef.address is 0.

Given that end() (in both Inflater and Deflater) can be overridden, and
the fact the javadoc of the end() doesn't mention any explicit
guarantees of the underlying state (zsRef.address for example), do you
think using something like a boolean "closed" flag (similar to what Remi
suggested) would be more robust?

-Jaikiran

Re: Inputs on patch for JDK-8225763? Inflater and Deflater should implement AutoCloseable

[Jaikiran Pai]

Hello Stuart,

Thank you for the detailed response. Comments inline.

On 28/06/19 2:48 AM, Stuart Marks wrote:
> On 6/26/19 9:28 PM, Jaikiran Pai wrote:
>> I am looking to contribute a patch for the enhancement noted in
>> https://bugs.openjdk.java.net/browse/JDK-8225763. The change itself
>> looks relatively straightforward to me and here's what I plan to do:
>>
>> 1. Have both java.util.zip.Inflater and java.util.zip.Deflater start
>> implementing the AutoCloseable interface
>>
>> 2. Have the close() method call the end() method
>>
>> 3. Add javadoc to the close() implementation to mention that the end()
>> method gets called
>>
>> 4. Add test(s) to verify that the close indeed actually "ends" the
>> inflater/deflater
>>
>> 5. This is more of a question - do we have to update the class level
>> javadoc of both these classes to make a mention that these classes have
>> started implementing the AutoCloseable starting version 14 (or whichever
>> version this change makes into) of Java?
>>
>> Any other inputs/suggestions on what else might be needed in this
>> change?
>
> Hi Jaikiran,
>
> Thanks for picking this up. There are some small wrinkles with this,
> but I hope nothing that will prevent it from happening.
>
>
> 2) Alan already mentioned this, but we need to consider the issue of
> idempotent close() or end() carefully. It's not strictly required, but
> it would be nice. I had taken a quick look at end() and I *thought* it
> was idempotent already, but a more careful analysis needs to be done. 

I did some checks in the current JDK code. From what I see, the Inflater
and Deflater do not have any subclasses within the JDK itself. So I
reviewed the end() method implementations of Inflater and Deflater.

The Inflater does the following:

public void end() {
    synchronized (zsRef) {
    zsRef.clean();
    input = ZipUtils.defaultBuf;
    inputArray = null;
    }
    }

The zfRef.clean() ends up calling the java.lang.ref.Cleanable#clean(),
which as per the javadoc says "Unregisters the cleanable and invokes the
cleaning action. The cleanable's cleaning action is invoked *at most
once* regardless of the number of calls to {@code clean}."

(emphasis added by me)

Based on this code in the Inflater#end() and the contract of
Cleanable#clean(), it appears that the Inflater#end() is effectively
idempotent.

Deflater#end() has similar code, except the inputArray = null. So that
too seems effectively idempotent. (side note - perhaps Deflater#end()
too should reset the inputArray to null?)

To be extra sure, I did a basic quick test by updating the TotalInOut
test to call the inflater/deflater end() methods more than once. No
issues with that change and test passed as expected.

Having said that, this only takes into account the JDK's implementation
of these methods and not of any sub-classes. Let's take a look at the
class level javadoc of Deflater as an example. It says:

* @apiNote
 * To release resources used by this {@code Deflater}, the {@link
#end()} method
 * should be called explicitly. Subclasses are responsible for the
cleanup of resources
 * acquired by the subclass. Subclasses that override {@link
#finalize()} in order
 * to perform cleanup should be modified to use alternative cleanup
mechanisms such
 * as {@link java.lang.ref.Cleaner} and remove the overriding {@code
finalize} method.

and now lets take the javadoc of its end method:

/**
 * Closes the compressor and discards any unprocessed input.
 *
 * This method should be called when the compressor is no longer
 * being used. Once this method is called, the behavior of the
 * Deflater object is undefined.
 */

Although there's an expectation of cleaning up resources in the end()
method by the sub-classes, there's no clear expectation on what's the
expected behaviour if end() is called more than once. In fact, the
javadoc of end() which states "undefined behaviour once end is called"
makes it such that, currently, the sub-classes could potentially have
issues if end() gets called more than once. So IMO, I don't think end()
can be expected to be idempotent. Of course, this is based on my limited
knowledge of this API (and JDK codebase in general) so it's possible I
may have missed something.


> Idempotent close is an issue when using multiple, wrapped resources,
> and where an outer close() closes the inner one as well.
>
> If Inflater were retrofitted to implement AC, oddly, the following
> would not require idempotent close:
>
>     try (var inf = new Inflater();
>  var iis = new InflaterInputStream(inputStream, inf)) {
>     ...
>     }
>
> The reason is that InflaterInputStream does NOT clo

Re: Inputs on patch for JDK-8225763? Inflater and Deflater should implement AutoCloseable

[Jaikiran Pai]

On 29/06/19 4:31 PM, Jaikiran Pai wrote:
> Hello Stuart,
>
> Thank you for the detailed response. Comments inline.
>
> On 28/06/19 2:48 AM, Stuart Marks wrote:
>> On 6/26/19 9:28 PM, Jaikiran Pai wrote:
>>> I am looking to contribute a patch for the enhancement noted in
>>> https://bugs.openjdk.java.net/browse/JDK-8225763. The change itself
>>> looks relatively straightforward to me and here's what I plan to do:
>>>
>>> 1. Have both java.util.zip.Inflater and java.util.zip.Deflater start
>>> implementing the AutoCloseable interface
>>>
>>> 2. Have the close() method call the end() method
>>>
>>> 3. Add javadoc to the close() implementation to mention that the end()
>>> method gets called
>>>
>>> 4. Add test(s) to verify that the close indeed actually "ends" the
>>> inflater/deflater
>>>
>>> 5. This is more of a question - do we have to update the class level
>>> javadoc of both these classes to make a mention that these classes have
>>> started implementing the AutoCloseable starting version 14 (or whichever
>>> version this change makes into) of Java?
>>>
>>> Any other inputs/suggestions on what else might be needed in this
>>> change?
>> Hi Jaikiran,
>>
>> Thanks for picking this up. There are some small wrinkles with this,
>> but I hope nothing that will prevent it from happening.
>>
>>
>> 2) Alan already mentioned this, but we need to consider the issue of
>> idempotent close() or end() carefully. It's not strictly required, but
>> it would be nice. I had taken a quick look at end() and I *thought* it
>> was idempotent already, but a more careful analysis needs to be done. 
> I did some checks in the current JDK code. From what I see, the Inflater
> and Deflater do not have any subclasses within the JDK itself. 

To be clear - I couldn't find any subclasses in the main source code of
JDK. There are a couple of subclasses in the tests
(ConstructInflaterOutput.java and ConstructDeflaterInput.java), but
those don't do much outside of the context of testing.

-Jaikiran

Re: Inputs on patch for JDK-8225763? Inflater and Deflater should implement AutoCloseable

[Jaikiran Pai]

. Right now, most operations seem to call ensureOpen(),
> which throws NPE if the object has been closed. But "undefined" allows
> anything to happen, including filling the output buffer with garbage.
> That seems wrong. We might not want to specify what exception is
> thrown, but we might want to specify that *AN* exception is thrown --
> which must be a RuntimeException, since most methods don't declare any
> checked exceptions.
>
> In any case, the clause would have to be updated to say something like
> "Once this Deflater (Inflater) is closed, any subsequent operations
> will ."
In my patch I have attempted to clarify the end() method by taking your
input.
>
> **
>
> If you think the issues are settled enough, maybe it's time for you to
> take a stab at a patch. 

The initial version of my patch is now ready at [1]. Here's a high level
summary of what's contained in this patch:

- The Inflater/Deflater class now implements AutoCloseable

- The class level javadoc of both these classes has been to updated to
use newer style of code, with try-with-resources, for the example
contained in that javadoc. The @apiNote in these class level javadoc has
also been updated to mention the use of close() method for releasing
resources.

- The javadoc of end() method in both these classes has been updated to
encourage the use of close() method instead of this one. In addition,
this javadoc has also been updated to replace the "undefined behaviour"
statement with a mention that the usage of the Inflater/Deflater
instance after a call to end() may throw an exception in those
subsequent usages. Please note that, there's no such explicit mention in
the javadoc of the (newly added) close() method because IMO, it isn't
needed for close() since I think it's kind of implied that a closed
resource can no longer be used for further operations.

- The new close() method has been added along with javadoc which uses an
@implSpec to state that it internally calls end()

- TotalInOut.java test has been updated to use the new
try-with-resources construct for the inflater and deflater it uses.

- A couple of new test classes have been added to test various scenarios
where close() and end() method get called. These test mostly focus on
ensuring that the close() and end(), either implicitly or explicitly,
get called the right number of times on the subclasses of Inflater/Deflater.

- I have run the tests under test/jdk/java/util/zip/ by running:

jtreg -jdk:build/macosx-x86_64-server-release/images/jdk
test/jdk/java/util/zip/

and they have come back passing (except for failures/errors in
java/util/zip/ZipFile/TestZipFile.java, java/util/zip/3GBZipFiles.sh and
java/util/zip/DataDescriptorSignatureMissing.java - those issues though
are unrelated to this change and expected to fail, based on what I see
in their test report logs)

- In addition, I have sneaked in an unrelated change in this patch, into
the Deflater.end() method:

 public void end() {
 synchronized (zsRef) {
+    // check if already closed
+    if (zsRef.address() == 0) {
+    return;
+    }
 zsRef.clean();
 input = ZipUtils.defaultBuf;
+    inputArray = null;
+    }

Unlike in the Inflater.end(), the Deflater.end() didn't previously have
the "inputArray = null". I have included it in this patch, since it
looks right to clean up that array too. If this isn't the right
time/patch to do it, please do let me know and I'll take that up separately.

Thank you very much for your inputs so far.

[1] http://cr.openjdk.java.net/~jpai/webrev/8225763/1/webrev/index.html

P.S: I can start a new official RFR thread in this mailing list, if
needed. Please do let me know.

-Jaikiran


>
> Note: US holiday weekend coming up; replies might take several days.
>
> s'marks
>
>
>
>
> On 6/29/19 4:16 AM, Jaikiran Pai wrote:
>>
>> On 29/06/19 4:31 PM, Jaikiran Pai wrote:
>>> Hello Stuart,
>>>
>>> Thank you for the detailed response. Comments inline.
>>>
>>> On 28/06/19 2:48 AM, Stuart Marks wrote:
>>>> On 6/26/19 9:28 PM, Jaikiran Pai wrote:
>>>>> I am looking to contribute a patch for the enhancement noted in
>>>>> https://bugs.openjdk.java.net/browse/JDK-8225763. The change itself
>>>>> looks relatively straightforward to me and here's what I plan to do:
>>>>>
>>>>> 1. Have both java.util.zip.Inflater and java.util.zip.Deflater start
>>>>> implementing the AutoCloseable interface
>>>>>
>>>>> 2. Have the close() method call the end() method
>>>>>
>>>>> 3. Add javadoc to the close() implementation to mention that the
>>>>> e

Re: Inputs on patch for JDK-8225763? Inflater and Deflater should implement AutoCloseable

[Jaikiran Pai]

Hello Lance,

On 03/07/19 11:27 PM, Lance Andersen wrote:
>  Just a thought below to consider or ignore ;-) 
>> On Jul 2, 2019, at 11:27 PM, Stuart Marks > > wrote:
>>
>> Hi Jaikiran,
>>
>> OK, good analysis. Rather a lot of issues for what seems like a
>> simple patch, eh?
>>
>> - Idempotency
>>
>> Yes, it looks to me like Inflater.end() and Deflater.end()
>> implementations are idempotent. As you point out, overrides by
>> subclasses might not be. We should be clear when we're talking about
>> the specific implementatations of the end() methods in the Deflater
>> and Inflater classes, or whether we're talking about the contracts
>> defined by these method specifications on these classes and subtypes.
>>
>> The behavior of an implementation in a class can be specified with
>> @implSpec without imposing this on the contract of subclasses. This
>> is useful for subclasses that need to know the exact behavior of
>> calling super.end(), and also for callers who know they have an
>> instance of a particular class and not a subclass.
>>
>> The upshot is that while we might not have the end() method's
>> contract specify idempotency, we can certainly say so in an
>> @implSpec, if doing this will help. I'm not sure we'll actually need
>> it in this case, but let's keep it in mind.
>>
>> - Subclasses
>>
>> I don't think there are any subclasses in the JDK, and I did some
>> searching and I didn't find any subclasses "in the wild" either. I
>> did find a few uses of these classes though. If these classes are
>> rarely subclassed, we might be able to get away with changing the
>> contract, though this does entail some risk.
>
>
> we could consider  leaving end() as is and just introduce close()
> which while it duplicates what end() does, it  reduces  the chance of
> any potential complaints.
>
The issue with that is for subclasses which have their own resource
cleanup code solely in their end() method. This will actually be the
case for any subclasses that are out there currently. When we introduce
close() API and users of Deflater/Inflater start using it, if we
duplicate the logic of end() into our close(), then (as noted by Stuart
too) the end() method of the subclasses will never get called and will
lead to a resource leak of any resources those subclasses are holding onto.


>>
>>
>> - Deprecation of end()
>
> I don’t think we need to deprecate, just add a note to its javadoc
> encouraging the use of close() going forward...

Agreed and done in the initial version of the proposed patch
http://cr.openjdk.java.net/~jpai/webrev/8225763/1/webrev/index.html


-Jaikiran

RFR [14] JDK-8225763 Inflater and Deflater should implement AutoCloseable

[Jaikiran Pai]

Can I please get a review and a sponsor for the patch that implements
the enhancement noted in https://bugs.openjdk.java.net/browse/JDK-8225763 ?

There has been a recent discussion about this change in the
core-libs-dev mailing list here[1]. The latest version of the patch for
this change is now available at [2].

Here's a summary of what's contained in this patch:

- The Inflater/Deflater class now implements AutoCloseable

- The class level javadoc of both these classes has been to updated to
use newer style of code, with try-with-resources, for the example
contained in that javadoc. The @apiNote in these class level javadoc has
also been updated to mention the use of close() method for releasing
resources.

- The javadoc of end() method in both these classes has been updated to
encourage the use of close() method instead of this one. It now also has
a @implSpec which states that it's a no-op if called more than once.

In addition, this javadoc has also been updated to replace the
"undefined behaviour" statement with a mention that the usage of the
Inflater/Deflater instance after a call to end() may throw an exception
in those subsequent usages. Please note that, there's no such explicit
mention in the javadoc of the (newly added) close() method because IMO,
it isn't needed for close() since I think it's kind of implied that a
closed resource can no longer be used for further operations.

- The new close() method has been added along with javadoc which uses an
@implSpec to state that it internally calls end()

- TotalInOut.java test has been updated to use the new
try-with-resources construct for the inflater and deflater it uses.

- A couple of new (testng) test classes have been added to test various
scenarios where close() and end() method get called. These test mostly
focus on ensuring that the close() and end(), either implicitly or
explicitly, get called the right number of times on the subclasses of
Inflater/Deflater.

- I have run the tests under test/jdk/java/util/zip/ by running:

jtreg -jdk:build/macosx-x86_64-server-release/images/jdk
test/jdk/java/util/zip/

and they have come back passing (except for failures/errors in
java/util/zip/ZipFile/TestZipFile.java, java/util/zip/3GBZipFiles.sh and
java/util/zip/DataDescriptorSignatureMissing.java - those issues though
are unrelated to this change and expected to fail, based on what I see
in their test report logs)

- In addition, I have sneaked in an unrelated change in this patch, into
the Deflater.end() method:

 public void end() {
 synchronized (zsRef) {
+    // check if already closed
+    if (zsRef.address() == 0) {
+    return;
+    }
 zsRef.clean();
 input = ZipUtils.defaultBuf;
+    inputArray = null;
+    }

Unlike in the Inflater.end(), the Deflater.end() didn't previously have
the "inputArray = null". I have included it in this patch, since it
looks right to clean up that array too. If this isn't the right
time/patch to do it, please do let me know and I'll take that up separately.


[1]
http://mail.openjdk.java.net/pipermail/core-libs-dev/2019-June/061096.html

[2] http://cr.openjdk.java.net/~jpai/webrev/8225763/2/webrev/

-Jaikiran

Re: Inputs on patch for JDK-8225763? Inflater and Deflater should implement AutoCloseable

[Jaikiran Pai]

Hello Lance,

On 08/07/19 11:16 PM, Lance Andersen wrote:
> Hi Jaikiran,
>
> Thank you for your efforts here.
>
>> On Jul 6, 2019, at 9:59 AM, Jaikiran Pai > <mailto:jai.forums2...@gmail.com>> wrote:
>>
>>>
>>>  - Idempotency
>>>
>>> Yes, it looks to me like Inflater.end() and Deflater.end()
>>> implementations are idempotent. As you point out, overrides by
>>> subclasses might not be. We should be clear when we're talking about
>>> the specific implementatations of the end() methods in the Deflater
>>> and Inflater classes, or whether we're talking about the contracts
>>> defined by these method specifications on these classes and subtypes.
>>>
>>> The behavior of an implementation in a class can be specified with
>>> @implSpec without imposing this on the contract of subclasses. This is
>>> useful for subclasses that need to know the exact behavior of calling
>>> super.end(), and also for callers who know they have an instance of a
>>> particular class and not a subclass.
>>>
>>> The upshot is that while we might not have the end() method's contract
>>> specify idempotency, we can certainly say so in an @implSpec, if doing
>>> this will help. I'm not sure we'll actually need it in this case, but
>>> let's keep it in mind.
>>
>> Thank you. Although not for end(), I have now used this @implSpec in the
>> first version of my patch[1].
>
> This should be done for end() as well to set expectations.  While
> close() is the preferred way to go moving forward, end() is not going
> anywhere and still needs to be a first class-citizen WRT documentation.

Done. I have added the @implSpec for end() too in the new updated
revision of the webrev. I have opened a separate RFR thread containing
that webrev.


>>>
>>> **
>>>
>>> If you think the issues are settled enough, maybe it's time for you to
>>> take a stab at a patch. 
>>
>> The initial version of my patch is now ready at [1]. Here's a high level
>> summary of what's contained in this patch:
>
> Please start a review request thread so it is easier to follow.  Once
> you do that, I will reply to it..

Done - I have now created a new RFR thread for this here
http://mail.openjdk.java.net/pipermail/core-libs-dev/2019-July/061229.html


>
> Also, reminder to update copyright dates.

The updated webrev in the RFR thread contains this update.


>
> For the tests, it would be best to add more comments.  Out of
> curiosity, was there a reason you chose not to use TestNG vs having
> just a main which invokes each test?

No specific reason. I'm still relatively new to the JDK codebase and
don't know when to use testng as against the regular main() driven
tests. I have now cleaned up the tests and converted them to testng in
the new webrev that I proposed.

-Jaikiran

Re: RFR [14] JDK-8225763 Inflater and Deflater should implement AutoCloseable

[Jaikiran Pai]

Hello Lance,

On 10/07/19 2:25 AM, Lance Andersen wrote:
> Hi Jaikiran,
>
> Again, thank you for your efforts here.
>
> Is there a CSR for this yet? 

There isn't one yet. I will need help on this part, since I haven't
created a CSR before. Is this something that I can create (I'm just a
"author" in the JDK project)? If yes, is there some specific things I
need to do?

I'll reply to the rest of the review comments soon. Thank you very much
for doing this review and offering to sponsor.

-Jaikiran


> This will need to approved prior to moving forward with committing the
> feature.
>
> I can sponsor once everything has been approved and finalized.
>
>
>
>
> ———
> @implSpec This method is a no-op if this compressor has already
> 886 * been previously closed,
>
> 
>
> Please remove “already” in both the close() and end() methods.  I
> believe the preference is the @implSpec and its relatives are on their
> own line as in https://openjdk.java.net/jeps/8068562 and was done for
> @apiNote earlier
>
>  - Undefined behavior after close()/end()
>
> I am not convinced the new wording is an improvement and I know Stuart
> was not thrilled with the existing wording.  However given the classes
> may be subclassed, I am not sure we  can truly specify the behavior
> which could be why the original authors used that wording.   Stuart
> thoughts?
>
>
> Outside of the @ImplSpec, I am not sure the initial wording for end()
> and close() really need to differ:
>
> end():
>
> Closes the decompressor and discards any unprocessed input.
>
> close():
>
> Releases resources held by this decompressor and discards any
> unprocessed input.This method should be called when the
> decompressor is no longer needed
>
>
>
>
>> On Jul 9, 2019, at 9:18 AM, Jaikiran Pai > <mailto:jai.forums2...@gmail.com>> wrote:
>>
>> Can I please get a review and a sponsor for the patch that implements
>> the enhancement noted in
>> https://bugs.openjdk.java.net/browse/JDK-8225763 ?
>>
>> There has been a recent discussion about this change in the
>> core-libs-dev mailing list here[1]. The latest version of the patch for
>> this change is now available at [2].
>>
>> Here's a summary of what's contained in this patch:
>>
>> - The Inflater/Deflater class now implements AutoCloseable
>>
>> - The class level javadoc of both these classes has been to updated to
>> use newer style of code, with try-with-resources, for the example
>> contained in that javadoc. The @apiNote in these class level javadoc has
>> also been updated to mention the use of close() method for releasing
>> resources.
>>
>> - The javadoc of end() method in both these classes has been updated to
>> encourage the use of close() method instead of this one. It now also has
>> a @implSpec which states that it's a no-op if called more than once.
>>
>> In addition, this javadoc has also been updated to replace the
>> "undefined behaviour" statement with a mention that the usage of the
>> Inflater/Deflater instance after a call to end() may throw an exception
>> in those subsequent usages. Please note that, there's no such explicit
>> mention in the javadoc of the (newly added) close() method because IMO,
>> it isn't needed for close() since I think it's kind of implied that a
>> closed resource can no longer be used for further operations.
>
> We need to be specific in close()  also for clarity
>>
>> - The new close() method has been added along with javadoc which uses an
>> @implSpec to state that it internally calls end()
>>
>> - TotalInOut.java test has been updated to use the new
>> try-with-resources construct for the inflater and deflater it uses.
>
> Please update @biug to include the bug number
>>
>> - A couple of new (testng) test classes have been added to test various
>> scenarios where close() and end() method get called. These test mostly
>> focus on ensuring that the close() and end(), either implicitly or
>> explicitly, get called the right number of times on the subclasses of
>> Inflater/Deflater.
>
> Overall they look OK.  In your tests, you are testing the number of
> calls for the sub-classes not for Deflate/Inflate so I would either
> update your comments to clarify that or pull them into their own test
> methods
>
>
> Again, thank you for your efforts here.
>
> Best
> Lance
> <http://oracle.com/us/design/oracle-email-sig-198324.gif>
> <http://oracle.com/us/design/oracle-email-sig-198324.gif><http://oracle.com/us/design/oracle-email-sig-198324.gif>
> <http://oracle.com/us/design/oracle-email-sig-198324.gif>Lance
> Andersen| Principal Member of Technical Staff | +1.781.442.2037
> Oracle Java Engineering 
> 1 Network Drive 
> Burlington, MA 01803
> lance.ander...@oracle.com <mailto:lance.ander...@oracle.com>
>
>
>

Re: RFR [14] JDK-8225763 Inflater and Deflater should implement AutoCloseable

[Jaikiran Pai]

Hello Lance,

On 10/07/19 2:25 AM, Lance Andersen wrote:
> ———
> @implSpec This method is a no-op if this compressor has already
> 886 * been previously closed,
>
> 
>
> Please remove “already” in both the close() and end() methods.

Done.


>  I believe the preference is the @implSpec and its relatives are on
> their own line as in https://openjdk.java.net/jeps/8068562 and was
> done for @apiNote earlier

Done.


>
> Outside of the @ImplSpec, I am not sure the initial wording for end()
> and close() really need to differ:
>
> end():
>
> Closes the decompressor and discards any unprocessed input.
>
> close():
>
> Releases resources held by this decompressor and discards any
> unprocessed input.This method should be called when the
> decompressor is no longer needed
>
>
I have now updated the javadoc of end() to be closer to the javadoc of
close().


>
>> - The javadoc of end() method in both these classes has been updated to
>> encourage the use of close() method instead of this one. It now also has
>> a @implSpec which states that it's a no-op if called more than once.
>>
>> In addition, this javadoc has also been updated to replace the
>> "undefined behaviour" statement with a mention that the usage of the
>> Inflater/Deflater instance after a call to end() may throw an exception
>> in those subsequent usages. Please note that, there's no such explicit
>> mention in the javadoc of the (newly added) close() method because IMO,
>> it isn't needed for close() since I think it's kind of implied that a
>> closed resource can no longer be used for further operations.
>
> We need to be specific in close()  also for clarity

I haven't updated this in the latest webrev version and will wait for us
to come to a decision on how we word it for end().


>>
>>
>> - TotalInOut.java test has been updated to use the new
>> try-with-resources construct for the inflater and deflater it uses.
>
> Please update @biug to include the bug number

Done.


>>
>> - A couple of new (testng) test classes have been added to test various
>> scenarios where close() and end() method get called. These test mostly
>> focus on ensuring that the close() and end(), either implicitly or
>> explicitly, get called the right number of times on the subclasses of
>> Inflater/Deflater.
>
> Overall they look OK.  In your tests, you are testing the number of
> calls for the sub-classes not for Deflate/Inflate so I would either
> update your comments to clarify that or pull them into their own test
> methods
>
I did not understand this. Did you mean I should update the @summary
part of these tests or was it the javadoc on these test methods?

The latest webrev with the above noted changes is available at
http://cr.openjdk.java.net/~jpai/webrev/8225763/3/webrev/

-Jaikiran

Re: RFR [14] JDK-8225763 Inflater and Deflater should implement AutoCloseable

[Jaikiran Pai]

On 09/07/19 8:44 PM, David Lloyd wrote:
> On Tue, Jul 9, 2019 at 8:19 AM Jaikiran Pai  wrote:
>> - In addition, I have sneaked in an unrelated change in this patch, into
>> the Deflater.end() method:
>>
>>  public void end() {
>>  synchronized (zsRef) {
>> +// check if already closed
>> +if (zsRef.address() == 0) {
>> +return;
>> +}
>>  zsRef.clean();
>>  input = ZipUtils.defaultBuf;
>> +inputArray = null;
>> +}
>>
>> Unlike in the Inflater.end(), the Deflater.end() didn't previously have
>> the "inputArray = null". I have included it in this patch, since it
>> looks right to clean up that array too. If this isn't the right
>> time/patch to do it, please do let me know and I'll take that up separately.
> This was probably my mistake.  The fix looks good to me!


Thank you David.

-Jaikiran

Re: RFR [14] JDK-8225763 Inflater and Deflater should implement AutoCloseable

[Jaikiran Pai]

Hello Lance,

On 12/07/19 5:53 AM, Lance Andersen wrote:
> Hi Jaikiran,
>> I have now updated the javadoc of end() to be closer to the javadoc
>> of close().
>>
>>
>
> Its better but end() should include the wording 
>
> 
> This method should be called when the compressor is no longer needed.
>
> ——
>
> It could read
>
> This method or close() should be called when the compressor ……
>
>
> Also, thinking about the following in end():
>
> ———
> * Use of {@link #close()} is encouraged instead of this method.
> ———
>
> This might be better served as an @apiNote so it stands out

You are right, the wording you use is better. I have now updated the
javadoc of end() to use this wording. Furthermore, I have moved that
above part to the @apiNote.


>>>
 - The javadoc of end() method in both these classes has been updated to
 encourage the use of close() method instead of this one. It now
 also has
 a @implSpec which states that it's a no-op if called more than once.

 In addition, this javadoc has also been updated to replace the
 "undefined behaviour" statement with a mention that the usage of the
 Inflater/Deflater instance after a call to end() may throw an exception
 in those subsequent usages. Please note that, there's no such explicit
 mention in the javadoc of the (newly added) close() method because IMO,
 it isn't needed for close() since I think it's kind of implied that a
 closed resource can no longer be used for further operations.
>>>
>>> We need to be specific in close()  also for clarity
>>
>> I haven't updated this in the latest webrev version and will wait for
>> us to come to a decision on how we word it for end().
>>
>
> OK

Given that we haven't yet come to a decision on how to word this, I went
ahead and attempted to improve this wording a bit. I have now moved it
into the @apiNote of both end() and close() javadoc and it now reads:

"Once the {@link #end()} or {@link #close()} have been called on a
compressor, the compressor may not be used for further operations. Doing
so may cause an exception to be thrown."

Of course, we can still change this to something else if this isn't yet
accurate enough.


>>>
>>> Overall they look OK.  In your tests, you are testing the number of
>>> calls for the sub-classes not for Deflate/Inflate so I would either
>>> update your comments to clarify that or pull them into their own
>>> test methods
>>>
>> I did not understand this. Did you mean I should update the @summary
>> part of these tests or was it the javadoc on these test methods?
>>
>>
> Sorry if my comment was confusing.  For example:
>
> 
> public void testCloseThenEnd() throws Exception {
>  119 final Deflater simpleDeflater = new Deflater();
>  120 closeThenEnd(simpleDeflater);
>  121 
>  122 final OverrideClose overridenClose = new OverrideClose();
>  123 closeThenEnd(overridenClose);
>  124 // make sure close was called once
>  125 assertEquals(overridenClose.numTimesCloseCalled, 1, "Unexpected 
> number of calls to close()");
>
> ——
>
> As there is not an assert() for simpleDeflater, which is
> understandable,  the comment describing the test is not quite accurate
> should be slightly updated

OK, I get it now, thank you. Indeed what you note makes sense. I have
now updated both these new tests to move out the
"simpleDeflater/Inflater" testing outside into a new test method of its
own (along the lines to what you suggested in the previous reply) and
added comment to that test method to explain what it does. Let me know
if this looks better.

The new updated webrev is at
http://cr.openjdk.java.net/~jpai/webrev/8225763/4/webrev/

Thank you for your inputs so far.

-Jaikiran

Re: RFR [14] JDK-8225763 Inflater and Deflater should implement AutoCloseable

[Jaikiran Pai]

Hello Peter,

On 13/07/19 2:50 PM, Peter Levart wrote:
> Hi Jaikiran,
>
> On 7/12/19 9:07 AM, Jaikiran Pai wrote:
>> The new updated webrev is at
>> http://cr.openjdk.java.net/~jpai/webrev/8225763/4/webrev/
>
>
> I don't know if there are any custom subclasses of Inflater/Deflater
> around and what they do, but hypothetically consider a subclass of
> Deflater similar to this:
>
> public class MyDeflater extends Deflater {
>     private final Object lock = new Object();
>
>     @Override
>     public void finish() {
>     synchronized (lock) {
>     super.finish();
>     // ... my code ...
>     }
>     }
>
>     @Override
>     public int deflate(byte[] output, int off, int len, int flush) {
>     synchronized (lock) {
>     // ... my code ...
>     int result = super.deflate(output, off, len, flush);
>     // ... my code ...
>     return result;
>     }
>     }
>
>     @Override
>     public int deflate(ByteBuffer output, int flush) {
>     synchronized (lock) {
>     // ... my code ...
>     int result = super.deflate(output, flush);
>     // ... my code ...
>     return result;
>     }
>     }
>
>     @Override
>     public void reset() {
>     synchronized (lock) {
>     super.reset();
>     // ... my code ...
>     }
>     }
>
>     @Override
>     public void end() {
>     synchronized (lock) {
>     super.end();
>     // ... my code ...
>     }
>     }
> }
>
>
> Currently, there's no problem with this subclass as Deflater never
> calls no overridable method while holding internal lock (zsref). The
> newly added close() method is different. It calls end() while holding
> a lock (zsref). Suppose that close() is called from one thread, while
> at the same time some other overridden method is called from another
> thread. Deadlock may happen.

I'm really glad you brought this up. When I added this end() within the
synchronized block of close(), I had the same question in mind and was
wondering if it's something that we should consider. But I never pursued
it enough to actual come up with a detailed analysis like the one you
have done. Thank you for this.


>
>
> In your patch you are trying hard to prevent calling end() from
> close() if end() has already been called (directly or indirectly
> through close()) because a hypothetical subclass of Deflater that
> overrides end() might not be prepared for end() to be called multiple
> times (i.e. there was no specification requiring end() to be
> idempotent). But even in your implementation, there is a concurrent
> race possible that may allow the overridden end() method to be entered
> twice. Consider the following scenario:
>
> Thread A: calls end() explicitly
>
> Thread B: calls close()
>
> There's nothing preventing Thread B from entering overridden part of
> end() while thread A is executing this same part of code. Threads
> synchronize only at the point where the overridden end() calls
> super.end():
>
> public class MyDeflater extends Deflater {
>     @Override
>     public void end() {
>         // ... this part of code may execute concurrently
>         super.end();
>         // ... this part of code may execute concurrently
>     }
>
Good point. Agreed and it does defeat the code that tries to avoid
having end() invoked more than once.


> Well, the subclass may have it's own synchronization in place if it is
> used in a concurrent scenario, like in above hypothetical example:
>
>     @Override
>     public void end() {
>     synchronized (lock) {
>             // ... this part of code may execute multiple times
>     super.end();
>             // ... this part of code may execute multiple times
>     }
>     }
>
> But such synchronization has the already mentioned dangerous property
> of causing deadlock when close() is called concurrently with end(). In
> addition it doesn't prevent parts of overridden end() method to be
> executed multiple times.
>
> The following implementation of close() is in no way less effective in
> preventing multiple executions of parts of code in overridden end()
> method:
>
>     public void close() {
>     synchronized (zsRef) {
>     // check if already closed
>     if (zsRef.address() == 0) {
>     return;
>     }
>     }
>     // call end() out of synchronized block to prevent
>     // deadlock situations in concurrent scenarios
>     end();
>     }
>
> ...but it is deadlo

java.lang.System's comment about initializeSystemClass

[Jaikiran Pai]

The java.lang.System has this code comment[1] which states:

"

/* Register the natives via the static initializer.
 *
 * VM will invoke the initializeSystemClass method to complete
 * the initialization for this class separated from clinit.
 * Note that to use properties set by the VM, see the constraints
 * described in the initializeSystemClass method.
 */

"

However, I can't find that "initializeSystemClass" method anywhere in
that class or any other class in the JDK code. Is that comment still
relevant or am I looking in the wrong places?

[1]
http://hg.openjdk.java.net/jdk/jdk/file/9f5b92d5a1b2/src/java.base/share/classes/java/lang/System.java#l97

-Jaikiran

Re: java.lang.System's comment about initializeSystemClass

[Jaikiran Pai]

On 18/10/19 12:39 PM, Alan Bateman wrote:
> On 18/10/2019 08:03, Jaikiran Pai wrote:
>>
>> However, I can't find that "initializeSystemClass" method anywhere in
>> that class or any other class in the JDK code. Is that comment still
>> relevant or am I looking in the wrong places?
>>
> It's stale, the equivalent of what was initializeSystemClass is
> initPhase1 now.
>
> -Alan

Thank you Alan. Should I create a JBS issue to track and have this changed?

-Jaikiran

Re: java.lang.System's comment about initializeSystemClass

[Jaikiran Pai]

On 18/10/19 4:51 PM, Alan Bateman wrote:
> On 18/10/2019 08:12, Jaikiran Pai wrote:
>> :
>> Thank you Alan. Should I create a JBS issue to track and have this
>> changed?
>>
> Yes, go ahead, we just missed updating this comment in JDK 9.

Done - https://bugs.openjdk.java.net/browse/JDK-8232617


-Jaikiran

RFR: JDK-8232879: (zipfs) Writing out data with ZipFileSystem leads to a CRC failure in the generated jar file

[Jaikiran Pai]

Can I please get a review and a sponsor for a potential fix for
JDK-8232879[1]? The patch is available as a webrev at [2].

What's happening in there is that the
jdk.nio.zipfs.ZipFileSystem.DeflatingEntryOutputStream is overriding the
one arg write(byte b) method and calling the super.write(b) and then
doing a crc.update. The super.write(b)
(java.util.zip.DeflaterOutputStream in this case) actually internally
calls the 3 arg write(b, offset, length) which is overriding by this
jdk.nio.zipfs.ZipFileSystem.DeflatingEntryOutputStream. In its
implementation of write(b, offset, length), in addition to (rightly)
calling super.write(b, offset, length), this method also updates the CRC
(again). So this ends up updating the CRC multiple times when the single
arg write is invoked.

The patch now removes this overridden implementation of write(b) in the
DeflatingEntryOutputStream so that the call is handled by the
java.util.zip.DeflaterOutputStream. Although there's no @implNote on
java.util.zip.DeflaterOutputStream#write(byte b) static that it's
(always) going to call the 3 arg write(b, offset, length) method, the
implementation as of now does indeed do that. So I guess, its probably
OK to rely on that knowledge and get rid of this overridden write(b)
method instead of coming up with a bit more complicated fix.

The patch also includes a jtreg testcase which reproduces this issues
and verifies the fix.

[1] https://bugs.openjdk.java.net/browse/JDK-8232879

[2] https://cr.openjdk.java.net/~jpai/webrev/8232879/1/webrev/

-Jaikiran

Re: RFR: JDK-8232879: (zipfs) Writing out data with ZipFileSystem leads to a CRC failure in the generated jar file

[Jaikiran Pai]

Thank you Lance.

-Jaikiran

On 24/10/19 1:17 AM, Lance Andersen wrote:
> Hi Jaikiran,
>
> I will take a look and once we are good with the review, I can sponsor it.
>
> Best
> Lance
>> On Oct 23, 2019, at 7:24 AM, Jaikiran Pai > <mailto:jai.forums2...@gmail.com>> wrote:
>>
>> Can I please get a review and a sponsor for a potential fix for
>> JDK-8232879[1]? The patch is available as a webrev at [2].
>>
>> What's happening in there is that the
>> jdk.nio.zipfs.ZipFileSystem.DeflatingEntryOutputStream is overriding the
>> one arg write(byte b) method and calling the super.write(b) and then
>> doing a crc.update. The super.write(b)
>> (java.util.zip.DeflaterOutputStream in this case) actually internally
>> calls the 3 arg write(b, offset, length) which is overriding by this
>> jdk.nio.zipfs.ZipFileSystem.DeflatingEntryOutputStream. In its
>> implementation of write(b, offset, length), in addition to (rightly)
>> calling super.write(b, offset, length), this method also updates the CRC
>> (again). So this ends up updating the CRC multiple times when the single
>> arg write is invoked.
>>
>> The patch now removes this overridden implementation of write(b) in the
>> DeflatingEntryOutputStream so that the call is handled by the
>> java.util.zip.DeflaterOutputStream. Although there's no @implNote on
>> java.util.zip.DeflaterOutputStream#write(byte b) static that it's
>> (always) going to call the 3 arg write(b, offset, length) method, the
>> implementation as of now does indeed do that. So I guess, its probably
>> OK to rely on that knowledge and get rid of this overridden write(b)
>> method instead of coming up with a bit more complicated fix.
>>
>> The patch also includes a jtreg testcase which reproduces this issues
>> and verifies the fix.
>>
>> [1] https://bugs.openjdk.java.net/browse/JDK-8232879
>>
>> [2] https://cr.openjdk.java.net/~jpai/webrev/8232879/1/webrev/
>>
>> -Jaikiran
>>
>>
>
> <http://oracle.com/us/design/oracle-email-sig-198324.gif>
> <http://oracle.com/us/design/oracle-email-sig-198324.gif><http://oracle.com/us/design/oracle-email-sig-198324.gif>
> <http://oracle.com/us/design/oracle-email-sig-198324.gif>Lance
> Andersen| Principal Member of Technical Staff | +1.781.442.2037
> Oracle Java Engineering 
> 1 Network Drive 
> Burlington, MA 01803
> lance.ander...@oracle.com <mailto:lance.ander...@oracle.com>
>
>
>

Re: RFR: JDK-8232879: (zipfs) Writing out data with ZipFileSystem leads to a CRC failure in the generated jar file

[Jaikiran Pai]

Hello Christoph,

Thank you for the very detailed review. Comments inline.

On 24/10/19 3:00 AM, Langer, Christoph wrote:
>
> For the path to the test file, you could simply do: final Path jarPath = 
> Paths.get("zipfs-crc-test.jar");
> The test is run in the scratch directory of jtreg, so no reason to go to 
> java.io.tmpdir.

Thank you. I wasn't aware of jtreg scratch dir. Does it get used for
every test or is there something specific to the location/type of this
test? The updated webrev now uses the above construct.

> You can also skip deletion of this test file in the finally block, as the 
> jtreg scratch directories will be wiped by jtreg eventually.
>
> For the existence check of the test file in line 62, you can simply use 
> Files.exists.

Indeed. Updated in new webrev.


> As for creating the zipfs Filesystem (line 68), you can simply use 
> FileSystems.newFileSystem(Path, Map), no need to mess around with URIs.
The FileSystems.newFileSystem(Path, Map) doesn't exist in Java 11[1]. Of
course, this specific test will be run against latest upstream, where
this new method exists. I actually copied over that usage from my Java
11 reproducer. But given that this issue isn't about creating the
FileSystem itself, I have taken your advice and changed this line in the
new webrev.
>
> Line 96, where construct the input stream and then in 97, the ZipInputStream, 
> I suggest you either put both into the try statement or you use 
> ZipInputStream zis = new ZipInputStream(Files.newInputStream(jarPath)) and 
> then rely on ZipInputStream cascading the close.
Done - updated in new webrev.
>
> And my last suggestion: you could statically import Assert.assertEquals to 
> shorten lines 105 and 106.

Done - updated in new webrev.

The updated webrev is here
https://cr.openjdk.java.net/~jpai/webrev/8232879/2/webrev/

[1]
https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/nio/file/FileSystems.html

-Jaikiran

Re: RFR: JDK-8232879: (zipfs) Writing out data with ZipFileSystem leads to a CRC failure in the generated jar file

[Jaikiran Pai]

Thank you for the review, Lance.

On 26/10/19 4:25 AM, Lance Andersen wrote:
>
> The change to Zip FS looks good.  I re-worked the test so that it also
> runs against ZipFile (which uses the CEN vs the LOC) and Zip FS in
> addition to ZipInputStream for extra coverage.
>
> The webrev can be found
> at: http://cr.openjdk.java.net/~lancea/8232879/webrev.00/index.html

Looks fine. A very minor note about
http://cr.openjdk.java.net/~lancea/8232879/webrev.00/test/jdk/jdk/nio/zipfs/CRCWriteTest.java.html

136 while ((ze = zis.getNextEntry()) != null) {
 137 assertNotNull(ze);

Looks like an oversight - that assertNotNull(ze) on 137 isn't needed due to the 
!= null check on 136.

-Jaikiran

Re: ZipFileSystem and the impact of JDK-8031748 on ordering of MANIFEST files in the created jars

[Jaikiran Pai]

Adding core-libs-dev, since this also involves java.util.jar APIs.

-Jaikiran

On 14/11/19 8:47 PM, Jaikiran Pai wrote:
> Please consider the code listed at the end of this mail. What it does is
> uses ZipFileSystem to create 2 jar files with identical content. foo.jar
> and bar.jar. Both these jar files contain a (random) text file and a
> META-INF/MANIFEST.MF.
>
> In case of foo.jar, the text file gets created first and then the
> META-INF/MANIFEST.MF and the filesystem finally gets closed
>
> In case of bar.jar, the META-INF/MANIFEST.MF gets created first and then
> the text file and the filesystem finally gets closed.
>
> Once both these (identical) jars are created, the JarInputStream class
> is then used to open these jars and get hold of the Manifest file. What
> results is - the JarInputStream returns a null Manifest for foo.jar (the
> one where the META-INF/MANIFEST.MF wasn't created first), whereas the
> JarInputStream for bar.jar rightly finds the Manifest and its correct
> content.
>
> First of all it's a surprise that the JarInputStream seemingly "loses"
> the Manifest if the META-INF/MANIFEST.MF wasn't created first. But given
> that it's already a known issue reported in JBS
> https://bugs.openjdk.java.net/browse/JDK-8031748, at least we (the
> libraries that create jar files know what to do or how to deal with it).
>
> However, when using something like a zipfs FileSystem, where the code
> which deals with writing out content to the filesystem using
> standard/basic java.nio.file.Path and outputstreams, its hard to keep
> track (within the libraries or user code) of the order in which the
> files get written out. So is there any way this (undocumented)
> requirement be implemented as an internal detail within the zipfs
> filesystem implementation, such that it orders the META-INF/MANIFEST.MF
> entry correctly? Or is there a way JarInputStream itself can be fixed to
> not mandate this requirement?
>
> This issue was reported in the Quarkus project
> https://github.com/quarkusio/quarkus/issues/5399 and a workaround has
> been proposed, but given how involved the code is (unrelated to this
> issue), it's going to become more and more difficult to manage this
> ordering.
>
>
> Code reproducing this issue follows:
>
> import java.io.*;
> import java.nio.file.*;
> import java.util.*;
> import java.util.jar.*;
> import java.net.*;
> import static java.nio.file.StandardOpenOption.*;
>
> public class ZipFSJar {
>     public static void main(final String[] args) throws Exception {
>         final Map options =
> Collections.singletonMap("create", "true");
>         final Path jarPath = Paths.get("foo.jar");
>         try (final FileSystem zipFs =
> FileSystems.newFileSystem(URI.create("jar:" + jarPath.toUri()), options)) {
>             // first write non-manifest content
>             writeTxtFile(zipFs.getPath("foo.txt"));
>             // now write manifest
>             Files.createDirectories(zipFs.getPath("META-INF"));
>         final Path manifestPath = zipFs.getPath("META-INF",
> "MANIFEST.MF");
>             writeManifest(manifestPath, "foo.bar.Baz");
>         }
>
>         // repeat for bar.jar but with manifest being written out first
>         final Path barJar = Paths.get("bar.jar");
>         try (final FileSystem zipFs =
> FileSystems.newFileSystem(URI.create("jar:" + barJar.toUri()), options)) {
>             Files.createDirectories(zipFs.getPath("META-INF"));
>         final Path manifestPath = zipFs.getPath("META-INF",
> "MANIFEST.MF");
>             // first write manifest content
>             writeManifest(manifestPath, "foo.bar.Baz");
>             // now write text file
>             writeTxtFile(zipFs.getPath("foo.txt"));
>         }
>
>         // now check the jar contents
>         final Path[] jars = new Path[] {jarPath, barJar};
>         for (final Path p : jars) {
>             try (InputStream fileInputStream = new
> FileInputStream(p.toFile());
>     final JarInputStream jaris = new
> JarInputStream(fileInputStream);) {
>         final Manifest manifest = jaris.getManifest();
>         if (manifest == null) {
>             System.err.println(p + " is missing the manifest file");
>         } else {
>             System.out.println(p + " has the manifest file");
>             final String mainClass =
> manifest.getMainAttributes().getValue(Attributes.Name.MAIN_CLASS);
>      

Re: ZipFileSystem and the impact of JDK-8031748 on ordering of MANIFEST files in the created jars

[Jaikiran Pai]

Hello Lance,

On 14/11/19 9:35 PM, Lance Andersen wrote:
> ...
>
> There is an existing Zip FS enhancement
> request, https://bugs.openjdk.java.net/browse/JDK-8211917. 

Thank you, I hadn't noticed this one before sending this mail. I'll keep
a watch on this one.

-Jaikiran

Class-Path (in jar file) semantics different between Java 11 and 13 (on Windows)?

[Jaikiran Pai]

Imagine 2 jar files. One called helloworld.jar which contains just a
single org.myapp.HelloWorld class which prints to System.out from its
main method. The other jar called manifest-cp-test.jar. This
manifest-cp-test.jar contains (only a) META-INF/MANIFEST.MF with the
following content:

Manifest-Version: 1.0
Class-Path: /C:/helloworld.jar
Main-Class: org.myapp.HelloWorld

So this manifest-cp-test.jar has a Main-Class entry which points to the
HelloWorld class that belongs in the other helloworld.jar file. Both the
helloworld.jar and the manifest-cp-test.jar reside at C:\ on a Windows
system.

When run using Java 11, this runs correctly and prints the HelloWorld
message:

C:\>jdk-11.0.2\bin\java -jar manifest-cp-test.jar
Hello World


However, when run using Java 13 (and even latest upstream Java 14) on
the same Windows system, this now runs into a ClassNotFoundException:

Java 13:

C:\>jdk-13.0.1\bin\java -jar manifest-cp-test.jar
Error: Could not find or load main class org.myapp.HelloWorld
Caused by: java.lang.ClassNotFoundException: org.myapp.HelloWorld


Java 14:

C:\>jdk-14\bin\java -jar manifest-cp-test.jar
Error: Could not find or load main class org.myapp.HelloWorld
Caused by: java.lang.ClassNotFoundException: org.myapp.HelloWorld


Adding the "-Djdk.net.URLClassPath.showIgnoredClassPathEntries=true"
debug flag to the java launch command shows:

C:\>jdk-13.0.1\bin\java
-Djdk.net.URLClassPath.showIgnoredClassPathEntries=true
-jar manifest-cp-test.jar
Class-Path entry: "/C:/helloworld.jar" ignored in JAR file
file:/C:/manifest-cp-
test.jar
Error: Could not find or load main class org.myapp.HelloWorld
Caused by: java.lang.ClassNotFoundException: org.myapp.HelloWorld

So this Class-Path entry is being ignored starting Java 13.

I compared the spec for Class-Path entry for Java 11[1] and Java 13[2]
and I do changes in that section. The Java 13 version doc has those 3
rules mentioned under "A Class-Path entry is valid if the following
conditions are true:". So all 3 rules should be satisfied? I guess that
then means that this style of Class-Path entry will not satisfy the "It
can be used to create a URL, by resolving it against the context JAR's
URL"? I haven't yet had the time to look into the code or read up a bit
more on what this rule actually means. Specifically what does resolve
mean here?

[1]
https://docs.oracle.com/en/java/javase/11/docs/specs/jar/jar.html#class-path-attribute

[2]
https://docs.oracle.com/en/java/javase/13/docs/specs/jar/jar.html#class-path-attribute

P.S: This is another case of Class-Path semantics which behave not just
differently in different versions of Java but different in different
subsystems of the same Java version itself. More about this has been
discussed recently in
https://mail.openjdk.java.net/pipermail/compiler-dev/2019-October/013766.html
on compiler-dev.

-Jaikiran

Re: Class-Path (in jar file) semantics different between Java 11 and 13 (on Windows)?

[Jaikiran Pai]

FWIW - this was reported by one of Quarkus project users here
https://github.com/quarkusio/quarkus/issues/5359

-Jaikiran

On 18/11/19 8:31 PM, Jaikiran Pai wrote:
> Imagine 2 jar files. One called helloworld.jar which contains just a
> single org.myapp.HelloWorld class which prints to System.out from its
> main method. The other jar called manifest-cp-test.jar. This
> manifest-cp-test.jar contains (only a) META-INF/MANIFEST.MF with the
> following content:
>
> Manifest-Version: 1.0
> Class-Path: /C:/helloworld.jar
> Main-Class: org.myapp.HelloWorld
>
> So this manifest-cp-test.jar has a Main-Class entry which points to the
> HelloWorld class that belongs in the other helloworld.jar file. Both the
> helloworld.jar and the manifest-cp-test.jar reside at C:\ on a Windows
> system.
>
> When run using Java 11, this runs correctly and prints the HelloWorld
> message:
>
> C:\>jdk-11.0.2\bin\java -jar manifest-cp-test.jar
> Hello World
>
>
> However, when run using Java 13 (and even latest upstream Java 14) on
> the same Windows system, this now runs into a ClassNotFoundException:
>
> Java 13:
>
> C:\>jdk-13.0.1\bin\java -jar manifest-cp-test.jar
> Error: Could not find or load main class org.myapp.HelloWorld
> Caused by: java.lang.ClassNotFoundException: org.myapp.HelloWorld
>
>
> Java 14:
>
> C:\>jdk-14\bin\java -jar manifest-cp-test.jar
> Error: Could not find or load main class org.myapp.HelloWorld
> Caused by: java.lang.ClassNotFoundException: org.myapp.HelloWorld
>
>
> Adding the "-Djdk.net.URLClassPath.showIgnoredClassPathEntries=true"
> debug flag to the java launch command shows:
>
> C:\>jdk-13.0.1\bin\java
> -Djdk.net.URLClassPath.showIgnoredClassPathEntries=true
> -jar manifest-cp-test.jar
> Class-Path entry: "/C:/helloworld.jar" ignored in JAR file
> file:/C:/manifest-cp-
> test.jar
> Error: Could not find or load main class org.myapp.HelloWorld
> Caused by: java.lang.ClassNotFoundException: org.myapp.HelloWorld
>
> So this Class-Path entry is being ignored starting Java 13.
>
> I compared the spec for Class-Path entry for Java 11[1] and Java 13[2]
> and I do changes in that section. The Java 13 version doc has those 3
> rules mentioned under "A Class-Path entry is valid if the following
> conditions are true:". So all 3 rules should be satisfied? I guess that
> then means that this style of Class-Path entry will not satisfy the "It
> can be used to create a URL, by resolving it against the context JAR's
> URL"? I haven't yet had the time to look into the code or read up a bit
> more on what this rule actually means. Specifically what does resolve
> mean here?
>
> [1]
> https://docs.oracle.com/en/java/javase/11/docs/specs/jar/jar.html#class-path-attribute
>
> [2]
> https://docs.oracle.com/en/java/javase/13/docs/specs/jar/jar.html#class-path-attribute
>
> P.S: This is another case of Class-Path semantics which behave not just
> differently in different versions of Java but different in different
> subsystems of the same Java version itself. More about this has been
> discussed recently in
> https://mail.openjdk.java.net/pipermail/compiler-dev/2019-October/013766.html
> on compiler-dev.
>
> -Jaikiran
>

Re: Class-Path (in jar file) semantics different between Java 11 and 13 (on Windows)?

[Jaikiran Pai]

Hello Alan,

On 18/11/19 9:17 PM, Alan Bateman wrote:
> On 18/11/2019 15:03, Jaikiran Pai wrote:
>> FWIW - this was reported by one of Quarkus project users here
>> https://github.com/quarkusio/quarkus/issues/5359
>>
> BTW: Do you know if this is a mistake in this project or something in
> a plugin used in its build? 

AFAIK, It actually wasn't a mistake but an intentional effort to make it
work on Windows. The linked thread on compiler-dev[1] has details
(especially the interpretation of "relative URLs") about why the
Manifest was constructed in this manner. The actual code which does this
construction, resides here[2] .


> There was an outreach effort to tack to down build tools and plugins
> that were incorrectly setting the attribute value to a file path
> instead of a URL.
>
Quarkus is a relatively new project and furthermore this specific code
is very new too (a few months old I think). So I think this never got
covered as part of the outreach efforts.


[1]
https://mail.openjdk.java.net/pipermail/compiler-dev/2019-October/013766.html

[2]
https://github.com/quarkusio/quarkus/blob/master/devtools/maven/src/main/java/io/quarkus/maven/DevMojo.java#L390

-Jaikiran

Re: RFR 8235361 : JAR Class-Path no longer accepts relative URLs encoding absolute Windows paths (e.g "/C:/...")

[Jaikiran Pai]

Thank you all for the real quick fix for these 2 issues.

In the context of Quarkus project, trying to get a decent workaround to
get past these issues across all prominent Java versions (and OS) was
starting to get a bit out of hand. But with this fixed consistently
within in the JDK now, it's much more comforting that the workarounds
that we have put in place in Quarkus won't need to last too long.

-Jaikiran

On 10/12/19 1:47 AM, Jonathan Gibbons wrote:
> I note that javac now uses the same definition of tryResolveFile in
> its handling of Class-Path manifest entries, and so the behavior for
> the compiler and runtime should now be aligned.
>
> -- Jon
>
>
> On 12/09/2019 12:10 PM, Brent Christian wrote:
>> Hi,
>>
>> As discussed[1] last month on this alias, JAR Class-Path entries[2]
>> that encode an absolute Windows path (including a drive letter, so
>> e.g. "/C:/path/to/file.jar") are ignored as of 8211941.
>>
>> Such entries are legal relative URLs, and should not be ignored.
>> Please review my fix to correct this.
>>
>> Issue:
>> https://bugs.openjdk.java.net/browse/JDK-8235361
>> Webrev:
>> http://cr.openjdk.java.net/~bchristi/8235361/webrev06-open/
>>
>> Thanks,
>> -Brent
>>
>> 1.
>> https://mail.openjdk.java.net/pipermail/core-libs-dev/2019-November/063491.html
>>
>> 2.
>> https://docs.oracle.com/en/java/javase/13/docs/specs/jar/jar.html#class-path-attribute
>>
>

RFR 7143743: (zipfs) Potential memory leak with zip provider

[Jaikiran Pai]

Can I please get a review and a sponsor for a patch which fixes
JDK-7143743[1]? The patch is hosted as a webrev at [2].

As noted in the JBS issue, the ZipFileSystem, even after being closed
can end up holding on to large amounts of memory. The root of the issue
is the "inodes" map which is maintained by the
jdk.nio.zipfs.ZipFileSystem class. This map holds on to "IndexNode"(s).
IndexNode(s) are added/updated/removed to/from this map as and when the
filesystem is used to add/update/remove files. One such IndexNode type
is the jdk.nio.zipfs.ZipFileSystem$Entry type and an instance of this
type will actually hold on to the underlying data of the file as a
byte[] (the member is called "bytes").

Once the instance of the ZipFileSystem is closed, this "inodes" map
isn't cleared and as a result, potentially, large amounts of data can
end up staying in the jdk.nio.zipfs.ZipFileSystem$Entry.bytes member(s).

The commit here fixes that issue by simply clearing the "inodes" map in
the jdk.nio.zipfs.ZipFileSystem.close() method. I have checked the usage
of the "inodes" map and from what I see, it's usage in various places is
guarded by "ensureOpen" checks, which means that once the ZipFileSystem
instance is closed, the contents of these "inodes" map is no longer
relevant and hence clearing it shouldn't cause any issues.

Given the nature of this issue, I haven't included a jtreg test for this
change. However, I have run the existing ZipFSTester.java testcase to
make sure no obvious regressions are introduced. That test passed
successfully with this change.

A program (a slightly modified version of the one available in the JBS
issue) is made available at the end of this mail to reproduce this issue
(and verify the fix). To run this program, pass it a path to a directory
(as first argument) which contains files with large sizes and a path to
a (non-existent) zip file that needs to be created (as second argument).

In my manual testing, I used a directory with 3 files of a total size of
around 831MB:

$ pwd

/home/me/testdir

$ ls -lh

total 1702360
-rw-r--r--@ 1 jaikiran  184M Oct 29 09:52 a
-rw-r--r--@ 1 jaikiran  258M Oct  9 19:52 b
-rw-r--r--@ 1 jaikiran  368M Dec 26 08:30 c

$ du -sh

831M .

Running the program resulted in:

$ java ZipFSLeak.java /home/me/testdir/ before-fix.zip

Zipping /home/me/testdir/ to before-fix.zip
Copied a total of 849617826 bytes from /home/me/testdir/
Running some GC ...
Memory in use (after GC): 853071256

(notice that the memory in use at the end of the program, after the
ZipFileSystem has been closed a while back, stays extremely high and
almost equal to the total bytes of the files that were added to the
ZipFileSystem).

Now after the fix, running the same program against the same directory
results in:

$ java ZipFSLeak.java /home/me/testdir/ after-fix.zip

Zipping /home/me/testdir/ to after-fix.zip
Copied a total of 849617826 bytes from /home/me/testdir/
Running some GC ...
Memory in use (after GC): 4769904

(notice the drastic improvement in the memory usage)

Following is the program used to reproduce the issue:


import java.io.IOException;
import java.net.URI;
import java.nio.file.DirectoryStream;
import java.nio.file.FileSystem;
import java.nio.file.FileSystems;
import java.nio.file.Files;
import java.nio.file.Path;
import java.util.HashMap;
import java.util.Map;


public class ZipFSLeak {

    public static void main(String[] args) throws IOException {
        // first arg is dir contatining (preferably) files of large sizes
        final Path srcDir = FileSystems.getDefault().getPath(args[0]);
        // second arg is path to the target zip which will be created by
this program
        final Path targetZip = FileSystems.getDefault().getPath(args[1]);
        System.out.println("Zipping " + srcDir + " to " + targetZip);

        final FileSystem zip = createZipFileSystem(targetZip, true);
        long totalCopiedSize = 0;
        try (final DirectoryStream dirStream =
Files.newDirectoryStream(srcDir);
                var zipFS = zip) {

            final Path zipRoot = zipFS.getPath("/");
            for (Path path : dirStream) {
                Files.copy(path,
zipRoot.resolve(path.getFileName().toString()));
                totalCopiedSize += path.toFile().length();
            }
        }

        System.out.println("Copied a total of " + totalCopiedSize + "
bytes from " + srcDir);
        System.out.println("Running some GC ...");
        // run some GC
        for (int i = 0; i < 10; i++) {
            Runtime.getRuntime().gc();
        }
        System.out.println("Memory in use (after GC): " +
(Runtime.getRuntime().totalMemory() - Runtime.getRuntime().freeMemory()));
    }

    private static FileSystem createZipFileSystem(Path path, boolean
create) throws IOException {
        final URI uri = URI.create("jar:file:" + path.toUri().getPath());

        final Map env = new HashMap<>();
        if (create) {
            env.put("create", "true");
        }
        return

Re: RFR 7143743: (zipfs) Potential memory leak with zip provider

[Jaikiran Pai]

Hello Alan,

On 11/01/20 3:37 pm, Alan Bateman wrote:
> On 11/01/2020 09:51, Jaikiran Pai wrote:
>> :
>>
>> The commit here fixes that issue by simply clearing the "inodes" map in
>> the jdk.nio.zipfs.ZipFileSystem.close() method. I have checked the usage
>> of the "inodes" map and from what I see, it's usage in various places is
>> guarded by "ensureOpen" checks, which means that once the ZipFileSystem
>> instance is closed, the contents of these "inodes" map is no longer
>> relevant and hence clearing it shouldn't cause any issues.
>>
> Clearing the inodes map should be okay for cases where something is
> holding a reference to a closed zip file system. However, you should
> look at beginWrite/endWrite so that all access to the map is
> consistently synchronized.
>
Thank you very much for that input - I hadn't considered the concurrency
aspect of it. Based on your input and after looking at the usage of the
"inodes", I have now updated the patch to use proper locks during the
clearing of the inodes. The updated webrev is available at
https://cr.openjdk.java.net/~jpai/webrev/7143743/2/webrev/

-Jaikiran

Re: RFR 7143743: (zipfs) Potential memory leak with zip provider

[Jaikiran Pai]

Thank you Lance.

-Jaikiran

On 12/01/20 2:26 am, Lance Andersen wrote:
> I am happy to sponsor this next week after providing time for
> additional review feedback and also sanity check it via Mach5
>
> Best
> Lance
>
>> On Jan 11, 2020, at 5:24 AM, Jaikiran Pai > <mailto:jai.forums2...@gmail.com>> wrote:
>>
>> Hello Alan,
>>
>> On 11/01/20 3:37 pm, Alan Bateman wrote:
>>> On 11/01/2020 09:51, Jaikiran Pai wrote:
>>>> :
>>>>
>>>> The commit here fixes that issue by simply clearing the "inodes" map in
>>>> the jdk.nio.zipfs.ZipFileSystem.close() method. I have checked the
>>>> usage
>>>> of the "inodes" map and from what I see, it's usage in various
>>>> places is
>>>> guarded by "ensureOpen" checks, which means that once the ZipFileSystem
>>>> instance is closed, the contents of these "inodes" map is no longer
>>>> relevant and hence clearing it shouldn't cause any issues.
>>>>
>>> Clearing the inodes map should be okay for cases where something is
>>> holding a reference to a closed zip file system. However, you should
>>> look at beginWrite/endWrite so that all access to the map is
>>> consistently synchronized.
>>>
>> Thank you very much for that input - I hadn't considered the concurrency
>> aspect of it. Based on your input and after looking at the usage of the
>> "inodes", I have now updated the patch to use proper locks during the
>> clearing of the inodes. The updated webrev is available at
>> https://cr.openjdk.java.net/~jpai/webrev/7143743/2/webrev/
>>
>> -Jaikiran
>
> <http://oracle.com/us/design/oracle-email-sig-198324.gif>
> <http://oracle.com/us/design/oracle-email-sig-198324.gif><http://oracle.com/us/design/oracle-email-sig-198324.gif>
> <http://oracle.com/us/design/oracle-email-sig-198324.gif>Lance
> Andersen| Principal Member of Technical Staff | +1.781.442.2037
> Oracle Java Engineering 
> 1 Network Drive 
> Burlington, MA 01803
> lance.ander...@oracle.com <mailto:lance.ander...@oracle.com>
>
>
>

Re: RFR 7143743: (zipfs) Potential memory leak with zip provider

[Jaikiran Pai]

Hello Christoph,

Setting inodes to null sounds fine to me and as you say since its usage
is already guarded by ensureOpen, IMO, it should be fine. I've now
updated the patch to set inodes to null in close() and the new updated
webrev is at https://cr.openjdk.java.net/~jpai/webrev/7143743/3/webrev/

-Jaikiran

On 13/01/20 12:26 pm, Langer, Christoph wrote:
> Hi,
>
> I'm wondering whether we shouldn't just do "inodes = null;"? I guess this is 
> cheaper and accesses to the inodes map on a closed filesystem object should 
> not happen anyway (e.g. all is guarded by ensureOpen()). Other than that, the 
> change looks fine.
>
> Cheers
> Christoph
>
>> -Original Message-
>> From: nio-dev  On Behalf Of Jaikiran
>> Pai
>> Sent: Samstag, 11. Januar 2020 11:24
>> To: Alan Bateman ; nio-...@openjdk.java.net
>> Cc: core-libs-dev@openjdk.java.net
>> Subject: Re: RFR 7143743: (zipfs) Potential memory leak with zip provider
>>
>> Hello Alan,
>>
>> On 11/01/20 3:37 pm, Alan Bateman wrote:
>>> On 11/01/2020 09:51, Jaikiran Pai wrote:
>>>> :
>>>>
>>>> The commit here fixes that issue by simply clearing the "inodes" map in
>>>> the jdk.nio.zipfs.ZipFileSystem.close() method. I have checked the usage
>>>> of the "inodes" map and from what I see, it's usage in various places is
>>>> guarded by "ensureOpen" checks, which means that once the
>> ZipFileSystem
>>>> instance is closed, the contents of these "inodes" map is no longer
>>>> relevant and hence clearing it shouldn't cause any issues.
>>>>
>>> Clearing the inodes map should be okay for cases where something is
>>> holding a reference to a closed zip file system. However, you should
>>> look at beginWrite/endWrite so that all access to the map is
>>> consistently synchronized.
>>>
>> Thank you very much for that input - I hadn't considered the concurrency
>> aspect of it. Based on your input and after looking at the usage of the
>> "inodes", I have now updated the patch to use proper locks during the
>> clearing of the inodes. The updated webrev is available at
>> https://cr.openjdk.java.net/~jpai/webrev/7143743/2/webrev/
>>
>> -Jaikiran

Re: RFR 7143743: (zipfs) Potential memory leak with zip provider

[Jaikiran Pai]

Looks fine Lance. I forgot about the copyright year, thank you for
taking care of that one too.

-Jaikiran

On 14/01/20 1:56 am, Lance Andersen wrote:
>
>
>> On Jan 13, 2020, at 1:53 PM, Alan Bateman > <mailto:alan.bate...@oracle.com>> wrote:
>>
>> On 13/01/2020 10:31, Jaikiran Pai wrote:
>>> Hello Christoph,
>>>
>>> Setting inodes to null sounds fine to me and as you say since its usage
>>> is already guarded by ensureOpen, IMO, it should be fine. I've now
>>> updated the patch to set inodes to null in close() and the new updated
>>> webrev is at https://cr.openjdk.java.net/~jpai/webrev/7143743/3/webrev/
>>>
>> This version looks good except it might be better if the comment just
>> says that it clears the inodes map to allow the keys/values be GC’ed.
>
> I revised the comment to:
>
> 
>
> $ hg diff
> *diff -r 9338d0f52b2e
> src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java*
> *--- a/src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java   
>   Mon Jan 13 11:51:45 2020 -0500*
> *+++ b/src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java   
>   Mon Jan 13 15:24:37 2020 -0500*
> @@ -1,5 +1,5 @@
>  /*
> - * Copyright (c) 2009, 2019, Oracle and/or its affiliates. All rights
> reserved.
> + * Copyright (c) 2009, 2020, Oracle and/or its affiliates. All rights
> reserved.
>   * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
>   *
>   * This code is free software; you can redistribute it and/or modify it
> @@ -490,6 +490,14 @@
>                  def.end();
>          }
>
>  
>
> +        beginWrite();                // lock and sync
> +        try {
> +            // Clear the map so that its keys & values can be garbage
> collected
> +            inodes = null;
> +        } finally {
> +            endWrite();
> +        }
> +
>          IOException ioe = null;
>          synchronized (tmppaths) {
>              for (Path p : tmppaths) {
> $
> ———
>
> I will push the change tomorrow barring any hiccups with Mach5 or
> additional comments….
>
> Best
> lance
>>
>> -Alan
>
> <http://oracle.com/us/design/oracle-email-sig-198324.gif>
> <http://oracle.com/us/design/oracle-email-sig-198324.gif><http://oracle.com/us/design/oracle-email-sig-198324.gif>
> <http://oracle.com/us/design/oracle-email-sig-198324.gif>Lance
> Andersen| Principal Member of Technical Staff | +1.781.442.2037
> Oracle Java Engineering 
> 1 Network Drive 
> Burlington, MA 01803
> lance.ander...@oracle.com <mailto:lance.ander...@oracle.com>
>
>
>

DriverManager.isDriverAllowed has an unintentional side-effect?

[Jaikiran Pai]

Right now within the implementation of APIs in java.sql.DriverManager
there are classloader checks to ensure whether the caller is allowed
access to registered drivers. For example the getDriver(String url)
API[1] calls the isDriverAllowed (private) method[2] to check if the
registered driver is allowed access from the caller's classloader. The
implementation of isDriverAllowed has this[3]:

aClass =  Class.forName(driver.getClass().getName(), true, classLoader);

Is it intentional to intialize that class by passing "true" to the
forName call? The reason I ask is the following scenario:

1. Imagine a multi classloader environment.

2. Consider postgres JDBC driver (from postgres.jar) gets loaded through
classloader C1 and is registered in DriverManager in context of C1.

3. Now consider a class A loaded in classloader C2. Let's say this C2
also has postgres.jar in its classpath but hasn't yet loaded any of the
classes from it yet nor have any calls to "registerDriver" been made by
any of the classes loaded by this C2.

4. Class A (in context of C2) now calls getDriver(String url) passing it
a JDBC url corresponding to postgres. This will result in a
SQLException("No suitable driver") exception and that's fine, because C2
isn't allowed access to driver registered in context of C1. However, now
consider the following code(again within class A in context of C2):

try {

    DriverManager.getDriver("jdbc:postgres:");

} catch (SQLException e) {

  // expected

  // now lets do the same call again

  driver = DriverManager.getDriver("jdbc:postgres:"); --> this one
passes

}

So what's being done is, the SQLException("no suitable driver") is
caught and the exact same call which triggered this issue, is called
again. This time the call passes and returns a JDBC driver corresponding
to the postgres driver.

Looking at the implementation of DriverManager (which I linked before),
I can understand why this happens but it just seems odd that the API
would behave in this manner (that too with no indication in the
documentation).

What really is happening here is that the current implementation of
isDriverAllowed, due to its usage of "true" to initialize the driver
class ends up triggering the Driver's static block (which as per the
JDBC spec) is expected/mandated to register with the DriverManager. As a
result, this call ends up registering the driver, now in the context of
C2 and as a result the subsequent calls to this (and other APIs) start
passing from classes loaded by C2 (like that class A).

Should this check in the isDriverAllowed, instead use "false" and not
trigger the intialization of the class?

[1]
https://hg.openjdk.java.net/jdk/jdk/file/5a3b04593405/src/java.sql/share/classes/java/sql/DriverManager.java#l266

[2]
https://hg.openjdk.java.net/jdk/jdk/file/5a3b04593405/src/java.sql/share/classes/java/sql/DriverManager.java#l280

[3]
https://hg.openjdk.java.net/jdk/jdk/file/5a3b04593405/src/java.sql/share/classes/java/sql/DriverManager.java#l555

-Jaikiran

Re: DriverManager.isDriverAllowed has an unintentional side-effect?

[Jaikiran Pai]

Hello Lance,

On 11/02/20 2:05 am, Lance Andersen wrote:
> Hi Jaikiran
>
>
>>
>> Should this check in the isDriverAllowed, instead use "false" and not
>> trigger the intialization of the class?
>
> As I mentioned above, this dates back to the very early days of JDBC
> and JDK 1.2.  Any changes in this area could be quite disruptive and
> would require extensive testing. 
>
That's a valid reason and I understand the unwillingness to change this.

On a related note, the ensureDriversInitialized method right now gets
run once per JVM. However, given that the rest of the DriverManager
deals with per classloader Driver(s), would it be right to somehow make
ensureDriversInitialized run once per classloader instance? That way,
this issue won't show up in first place, given that
ensureDriversInitialized would have ensured that any drivers available
in that classloader would have been loaded during the first call from
class A (in that example).

-Jaikiran

Re: RFR: 8211917: (zipfs) Creating or updating a JAR file system should put the MANIFEST.MF at the start

[Jaikiran Pai]

Hello Lance,

On 05/02/20 3:13 am, Lance Andersen wrote:
> Hi Jaikiran,
>
> Thank you again for tackling this feature request.
>
> Overall, I think this looks OK.
>
> In ZipFileSystem.java, I would reverse the if statement given a
> MANiFEST.MF present is going to be the exception vs the norm.  Perhaps
> something similar to:
>
> 
> if(manifestInode == null || manifestProcessed) {
> inode = inodeIterator.next();
> if(inode == manifestInode)
> continue;
> } else {
> inode = manifestInode;
> manifestProcessed = true;
> }
> —
>
Done.


> A few comments/suggestions on the test:
>
>   * I would prefer  that the tests use the newer FileSystems::
> newFileSystem methods for the patch to the current openjdk repository
>
Done - updated the testcase to use the newer available APIs.


>   * Please use Map.of() vs Collections.xxxMap
>
Done.


>   * We should test with STORED and DEFLATED specified for compression.  
>   * I would look at the CopyMoveTest and leverage the Entry class and
> verify method in this test.  This will keep the tests looking
> somewhat similar
>
Done - the testcase now tests the default (== DEFLATED), STORED and
(explicit) DEFLATED compression methods.


>   * Please add a test which copies the Manifest from an OS file to the JAR
>
Done. New testManifestCopiedFromOSFile test method added.


>   * I would consider adding a test which creates a JAR with a Manifest
> and then uses Files::copy to create a another JAR from the
> original JAR
>
Done. New testDuplicateJar test method added.


>   * I would consider a test which creates the JAR via  the jar
> tool(using the ToolProvider API) and then updates the JAR via ZipFS
>
Done. New testJarToolGeneratedJarWithManifest added.


>   * You may want to consider executing the JAR if you are setting the
> main class attribute see the LargeEntriesTest as an example
>
In my initial version, I included the Main-Class manifest attribute only
to make sure the manifest file that is being verified is indeed the one
we had added in the tests. I did not have any intention of testing the
"Main-Class" semantics. In this newer updated version of the test case,
I decided to just remove the "Main-Class" and instead use a dummy
manifest attribute that I just check for equality. I decided to remove
the "Main-Class" attribute since I didn't want this test to do too many
things. Let me know if you prefer that the Main-Class to stay and be
verified that it can be launched.

All these changes are now part of the new webrev which is at
https://cr.openjdk.java.net/~jpai/webrev/8211917/2/webrev/

-Jaikiran

Re: RFR: 8211917: (zipfs) Creating or updating a JAR file system should put the MANIFEST.MF at the start

[Jaikiran Pai]

I realized that the verify() method in the testcase can include a couple
of more tests while dealing with the JarInputStream. So I've updated
that method and created a fresh webrev which is available at
https://cr.openjdk.java.net/~jpai/webrev/8211917/3/webrev/

-Jaikiran

On 11/02/20 10:03 pm, Jaikiran Pai wrote:
>
> Hello Lance,
>
> On 05/02/20 3:13 am, Lance Andersen wrote:
>> Hi Jaikiran,
>>
>> Thank you again for tackling this feature request.
>>
>> Overall, I think this looks OK.
>>
>> In ZipFileSystem.java, I would reverse the if statement given a
>> MANiFEST.MF present is going to be the exception vs the norm.
>>  Perhaps something similar to:
>>
>> 
>> if(manifestInode == null || manifestProcessed) {
>> inode = inodeIterator.next();
>> if(inode == manifestInode)
>> continue;
>> } else {
>> inode = manifestInode;
>> manifestProcessed = true;
>> }
>> —
>>
> Done.
>
>
>> A few comments/suggestions on the test:
>>
>>   * I would prefer  that the tests use the newer FileSystems::
>> newFileSystem methods for the patch to the current openjdk repository
>>
> Done - updated the testcase to use the newer available APIs.
>
>
>>   * Please use Map.of() vs Collections.xxxMap
>>
> Done.
>
>
>>   * We should test with STORED and DEFLATED specified for compression.  
>>   * I would look at the CopyMoveTest and leverage the Entry class and
>> verify method in this test.  This will keep the tests looking
>> somewhat similar
>>
> Done - the testcase now tests the default (== DEFLATED), STORED and
> (explicit) DEFLATED compression methods.
>
>
>>   * Please add a test which copies the Manifest from an OS file to
>> the JAR
>>
> Done. New testManifestCopiedFromOSFile test method added.
>
>
>>   * I would consider adding a test which creates a JAR with a
>> Manifest and then uses Files::copy to create a another JAR from
>> the original JAR
>>
> Done. New testDuplicateJar test method added.
>
>
>>   * I would consider a test which creates the JAR via  the jar
>> tool(using the ToolProvider API) and then updates the JAR via ZipFS
>>
> Done. New testJarToolGeneratedJarWithManifest added.
>
>
>>   * You may want to consider executing the JAR if you are setting the
>> main class attribute see the LargeEntriesTest as an example
>>
> In my initial version, I included the Main-Class manifest attribute
> only to make sure the manifest file that is being verified is indeed
> the one we had added in the tests. I did not have any intention of
> testing the "Main-Class" semantics. In this newer updated version of
> the test case, I decided to just remove the "Main-Class" and instead
> use a dummy manifest attribute that I just check for equality. I
> decided to remove the "Main-Class" attribute since I didn't want this
> test to do too many things. Let me know if you prefer that the
> Main-Class to stay and be verified that it can be launched.
>
> All these changes are now part of the new webrev which is at
> https://cr.openjdk.java.net/~jpai/webrev/8211917/2/webrev/
>
> -Jaikiran
>

Re: DriverManager.isDriverAllowed has an unintentional side-effect?

[Jaikiran Pai]

Hello Lance,

On 13/02/20 12:22 am, Lance Andersen wrote:
> Hi Jaikiran
>
>> On Feb 10, 2020, at 10:05 PM, Jaikiran Pai > <mailto:jai.forums2...@gmail.com>> wrote:
>>
>> Hello Lance,
>>
>> On 11/02/20 2:05 am, Lance Andersen wrote:
>>> Hi Jaikiran
>>>
>>>
>>>>
>>>> Should this check in the isDriverAllowed, instead use "false" and not
>>>> trigger the intialization of the class?
>>>
>>> As I mentioned above, this dates back to the very early days of JDBC
>>> and JDK 1.2.  Any changes in this area could be quite disruptive and
>>> would require extensive testing. 
>>>
>> That's a valid reason and I understand the unwillingness to change this.
>>
>> On a related note, the ensureDriversInitialized method right now gets
>> run once per JVM. However, given that the rest of the DriverManager
>> deals with per classloader Driver(s), would it be right to somehow make
>> ensureDriversInitialized run once per classloader instance? That way,
>> this issue won't show up in first place, given that
>> ensureDriversInitialized would have ensured that any drivers available
>> in that classloader would have been loaded during the first call from
>> class A (in that example).
>>
> This behavior also  dates back as well to JDK 1.2 and could be
> disruptive given how many years this code has been in place.
>
> It might be worth us  considering an alternative to DriveManager at
> some point allowing us to address some of  these limitations. 

Thank you for checking, Lance. I understand why it's not desirable to
change it at this point.


>
> In the meantime, you could choose to use a DataSource vs a Driver
> which if the DataSource is  implemented correctly does not rely on
> DriverManager.
>
>
For context, I happened to notice these issues in the DriverManager when
I was looking into this Quarkus reported issue
https://github.com/quarkusio/quarkus/issues/7079. There's anyway a
workaround for it https://github.com/quarkusio/quarkus/pull/7089 so it's
not that big an issue.

-Jaikiran

Re: RFR: 8211917: (zipfs) Creating or updating a JAR file system should put the MANIFEST.MF at the start

[Jaikiran Pai]

Hello Lance,

On 15/02/20 2:27 am, Lance Andersen wrote:
> Hi Jaikiran,
>
> I think the changes to ZipFileSystem are OK.
>
> The test overall is good.  I am going to streamline it a bit and
> remove the long lines (we try to keep lines to around 80 characters).
>
I'll keep that in mind for future changes. Thank you for taking care of
this.

-Jaikiran

Re: RFR(s): 8240235: jdk.test.lib.util.JarUtils updates jar files incorrectly

[Jaikiran Pai]

On 03/03/20 12:45 pm, Langer, Christoph wrote:
> Unfortunately I don't get the mails from Martin ☹

Same for me. They always end up in spam folder which I usually check
once a month. I see that there's already
https://bugs.openjdk.java.net/browse/JDK-8213225 which seems to be the
same issue.

-Jaikiran

Re: RFR: 8211917: (zipfs) Creating or updating a JAR file system should put the MANIFEST.MF at the start

[Jaikiran Pai]

Hello Lance,

On 28/02/20 2:41 am, Lance Andersen wrote:
> Hi Christoph,
>
> I have cleaned up, re-vamped and added more coverage to the test.  As
> part of this I started to lay the foundation for removing some of the
> duplicate code as part of continued clean up and enhanced coverage
> going forward
>
> The revised webrev can be found
> at: http://cr.openjdk.java.net/~lancea/8211917/webrev.00/index.html

This all looks good to me (of course, I'm not an official reviewer). The
new base testcase for zipfs related testing is definitely going to help
in future fixes/enhancements.

I just had one question in there:

+++ new/test/jdk/jdk/nio/zipfs/testng/util/ZipFsBaseTest.java   
...

+{Map.of("create", "true", "noCompression", "true"),
+ZipEntry.STORED},
+{Map.of("create", "true", "noCompression", "false"),
+ZipEntry.DEFLATED}

From what I had read in the javadoc of the private method 
getDefaultCompressionMethod in jdk.nio.zipfs.ZipFileSystem,
the "noCompression" property is only there for backward compatibility reasons 
and the new way
of configuring this semantic is the use of "compressionMethod" property (with 
value of either STORED
or DEFLATED). Is the use of "noCompression" in this base test class intentional 
or is it just a personal preference?
I'm fine either way, but wanted to know if I should stick to this form in any 
future test cases.

-Jaikiran

Re: Odd result with canonical paths with intermixed usage of java.io.File and java.nio.file.Files APIs

[Jaikiran Pai]

Moved this thread from discussions mailing list[1] to here. Comments inline.


On 11/12/17 7:55 PM, Alan Bateman wrote:

On 11/12/2017 14:06, Jaikiran Pai wrote:

:

So a few related questions that I have are:

    1. Is this inconsistency an expected behaviour or is this a bug?
    2. If this is an expected behaviour, then would it be a better 
idea (as an application developer) to use Path.toRealPath[2] instead 
of using the File.getCanonicalPath()? Are these 2 APIs semantically 
equivalent? The File.getCanonicalPath() talks about the canonical 
path being "unique" paths but the Path.toRealPath has no such mentions.


The correctness issues with the canonicalization cache is a long 
standing issue [1]. You can workaround it by running with 
-Dsun.io.useCanonCaches=false as I think you have found. The goal is 
to eventually disable and remove it. The first steps to get there 
happened in JDK 9 when FilePermission was changed to not canonicalize 
by default (FilePermission was the original motivation for this cache).


If you have follow-up questions then please bring the thread to 
core-libs-dev.


-Alan

[1] https://bugs.openjdk.java.net/browse/JDK-7066948
Thanks Alan. Given that the cache itself is being planned to be 
eventually removed, that answers the main part of my question and I can 
workaround this issue in a couple of ways (disabling the cache using 
that system property is one way) in the application, till that time.


The only remaining part that I'm curious about is this:

> ... would it be a better idea (as an application developer) to use 
Path.toRealPath[2] instead of using the File.getCanonicalPath()? Are 
these 2 APIs semantically equivalent? The File.getCanonicalPath() talks 
about the canonical path being "unique" paths but the Path.toRealPath 
has no such mentions.



[1] http://mail.openjdk.java.net/pipermail/discuss/2017-December/004655.html


-Jaikiran

Re: Odd result with canonical paths with intermixed usage of java.io.File and java.nio.file.Files APIs

[Jaikiran Pai]

On 11/12/17 9:43 PM, Alan Bateman wrote:

On 11/12/2017 15:46, Jaikiran Pai wrote:
Thanks Alan. Given that the cache itself is being planned to be 
eventually removed, that answers the main part of my question and I 
can workaround this issue in a couple of ways (disabling the cache 
using that system property is one way) in the application, till that 
time.


The only remaining part that I'm curious about is this:

> ... would it be a better idea (as an application developer) to use 
Path.toRealPath[2] instead of using the File.getCanonicalPath()? Are 
these 2 APIs semantically equivalent? The File.getCanonicalPath() 
talks about the canonical path being "unique" paths but the 
Path.toRealPath has no such mentions.
The main semantic difference is that toRealPath method can only return 
the real path of existing file (the file must exist). I don't know if 
that helps with your scenario or not.
It won't help in the specific case where I was thinking of using this, 
but it's a good detail to know. Thank you.


-Jaikiran

InetAddress.getByName/getAllByName for empty host string

[Jaikiran Pai]

The javadoc of InetAddress.getByName(host) and getAllByName(host) states 
that:


If the host is null then an InetAddress representing an address of the 
loopback interface is returned.


For non-null values the javadoc explains what the implementation does. 
However, there seems to be no mention of what it does with an empty 
string (length == 0) value for the host param. Right now, the 
implementation seems to treat an empty value the same way it treats the 
host == null case and returns an InetAddress representing the loopback 
address.


Consider the following example:

public class InetAddressTest {
    public static void main(final String[] args) throws Exception {
        System.out.println("InetAddress.getByName() for empty string 
returns " + java.net.InetAddress.getByName(""));
        System.out.println("InetAddress.getAllByName() for empty string 
returns "
            + 
java.util.Arrays.toString(java.net.InetAddress.getAllByName("")));


    }
}

This outputs:

InetAddress.getByName() for empty string returns localhost/127.0.0.1
InetAddress.getAllByName() for empty string returns [localhost/127.0.0.1]


Is it intentional for these APIs to behave this way for empty string? If 
so, should the javadoc be updated to explicitly state this behaviour?



-Jaikiran

Re: InetAddress.getByName/getAllByName for empty host string

[Jaikiran Pai]

Hi Chris,

Thank you creating that JIRA.

If the fix involves just updating the javadoc, is this something that 
youwould like me to contribute as a patch? I have a signed and approved 
OCA, but I will need a sponsor if I do come up with the patch.


-Jaikiran


On 13/04/18 8:41 PM, Chris Hegarty wrote:

Hi Jaikiran,

On 13/04/18 15:29, Jaikiran Pai wrote:
The javadoc of InetAddress.getByName(host) and getAllByName(host) 
states that:


If the host is null then an InetAddress representing an address of 
the loopback interface is returned.


For non-null values the javadoc explains what the implementation 
does. However, there seems to be no mention of what it does with an 
empty string (length == 0) value for the host param. Right now, the 
implementation seems to treat an empty value the same way it treats 
the host == null case and returns an InetAddress representing the 
loopback address.


Consider the following example:

public class InetAddressTest {
 public static void main(final String[] args) throws Exception {
 System.out.println("InetAddress.getByName() for empty string 
returns " + java.net.InetAddress.getByName(""));
 System.out.println("InetAddress.getAllByName() for empty 
string returns "
 + 
java.util.Arrays.toString(java.net.InetAddress.getAllByName("")));


 }
}

This outputs:

InetAddress.getByName() for empty string returns localhost/127.0.0.1
InetAddress.getAllByName() for empty string returns 
[localhost/127.0.0.1]



Is it intentional for these APIs to behave this way for empty string?


Yes.


If so, should the javadoc be updated to explicitly state this behaviour?


Yeah, probably.

The following JIRA issue has been filed to track this:
  https://bugs.openjdk.java.net/browse/JDK-8201545

-Chris.

Re: InetAddress.getByName/getAllByName for empty host string

[Jaikiran Pai]

Hi Chris,

On 16/04/18 9:33 PM, Chris Hegarty wrote:

Jaikiran,

On 13/04/18 16:29, Jaikiran Pai wrote:

Hi Chris,

Thank you creating that JIRA.

If the fix involves just updating the javadoc, is this something that 
youwould like me to contribute as a patch? I have a signed and 
approved OCA, but I will need a sponsor if I do come up with the patch.


A patch containing the specification clarification
I'm a bit new to the OpenJDK project - does the specification 
clarification (at least in the context of this issue) mean anything more 
than updating the javadoc?



and an
update to an existing test to cover said specification
clarification would be helpful.

I have started working on a test.



The change will require a CSR, but I can do that on your
behalf.
Thank you, can you please file one (if it can be done before a patch is 
submitted)? I'm in the process of creating the patch and will send it 
out to this list soon.


-Jaikiran

Re: InetAddress.getByName/getAllByName for empty host string

[Jaikiran Pai]

On 20/04/18 11:24 AM, Alan Bateman wrote:

On 20/04/2018 05:37, Jaikiran Pai wrote:
 I'm in the process of creating the patch and will send it out to 
this list soon.
Best to send it to the net-dev list as this is where the java.net API 
is mostly maintained.


Will do. Thank you.

-Jaikiran

[PATCH] JDK-8159797 Throw a right IllegalArgumentException from the bytecode generated by Method/ConstructorAccessor

[Jaikiran Pai]

Hi,

This mail contains a potential patch to the issue[1] that I had reported 
a couple of years back. The issue is still valid and reproducible with 
latest upstream JDK.


In brief, if a certain code has:

public void doSomething(String foo) {...}

and then it gets invoked through reflection:

thatMethod = // get hold of that method through reflection

thatMethod.invoke(validObjectInstance, null);

then as per the javadoc of the Method#invoke[2] you would expect an 
IllegalArgumentException (since the method expects 1 parameter and we 
are sending none). This does throw an IllegalArgumentException, but when 
the invocation happens through a (dynamically generated) MethodAccessor, 
instead of a native MethodAccessor, the IllegalArgumentException that 
gets thrown is due to a NPE that happens and the NPE's toString() output 
is contained as a message of the IllegalArgumentException, as noted in 
the JIRA. This happens even for Constructor invocations, through 
ConstructorAccessor.


The commit in the attached patch, adds bytecode instructions to check if 
the method arguments being passed is null and if so, doesn't attempt an 
arraylength instruction and instead just stores 0 on to the stack. This 
prevents the NullPointerException being reported, enclosed as a message 
within the IllegalArgumentException and instead throws back a proper 
IllegalArgumentException (as you would expect if the invocation had 
happened over a native MethodAccessor).


The goal of the patch _isn't_ to match the exception message with what 
the native MethodAccessor throws but just prevent the NPE from being 
playing a role in the IllegalArgumentException. i.e. this change 
_doesn't_ throw a new IllegalArgumentException("wrong number of arguments").


The patch also contains a new testcase which reproduces this against the 
current upstream and verifies this patch works.


This is the first time I'm fiddling with bytecode generation, so I would 
appreciate some feedback on the changed bytecode and if there's a 
different/better way to do it. Furthermore, although I do have a signed 
and approved OCA, I will need a sponsor for this patch. Anyone willing 
to review and sponsor, please?


[1] https://bugs.openjdk.java.net/browse/JDK-8159797

[2] 
https://docs.oracle.com/javase/10/docs/api/java/lang/reflect/Method.html#invoke(java.lang.Object,java.lang.Object...)


-Jaikiran

# HG changeset patch
# User Jaikiran Pai 
# Date 1525350350 -19800
#  Thu May 03 17:55:50 2018 +0530
# Node ID 2b535248cf02d90f6fea1c89150a277f16954c04
# Parent  7379e6f906aeb6e69ed8eccda9de285e606ab1ff
JDK-8159797 Prevent NPE from the generated Method/ConstructorAccessor when 
invoking with incorrect number of arguments

diff --git 
a/src/java.base/share/classes/jdk/internal/reflect/ClassFileConstants.java 
b/src/java.base/share/classes/jdk/internal/reflect/ClassFileConstants.java
--- a/src/java.base/share/classes/jdk/internal/reflect/ClassFileConstants.java
+++ b/src/java.base/share/classes/jdk/internal/reflect/ClassFileConstants.java
@@ -137,4 +137,7 @@
 
 // Access flags
 public static final short ACC_PUBLIC = (short) 0x0001;
+
+// push int constant to stack
+public static final byte opc_iconst_0   = (byte) 0x03;
 }
diff --git 
a/src/java.base/share/classes/jdk/internal/reflect/MethodAccessorGenerator.java 
b/src/java.base/share/classes/jdk/internal/reflect/MethodAccessorGenerator.java
--- 
a/src/java.base/share/classes/jdk/internal/reflect/MethodAccessorGenerator.java
+++ 
b/src/java.base/share/classes/jdk/internal/reflect/MethodAccessorGenerator.java
@@ -474,7 +474,13 @@
 //   aload_2 | aload_1 (Method | Constructor)
 //   ifnull 
 // aload_2 | aload_1
+// ifnull 
+// aload_2 | aload_1
 // arraylength
+// goto 
+// 
+// iconst_0
+// 
 // sipush 
 // if_icmpeq 
 // new 
@@ -496,7 +502,22 @@
 } else {
 cb.opc_aload_2();
 }
+Label nullArgsLabel = new Label();
+Label argCountCheckLabel = new Label();
+
+cb.opc_ifnull(nullArgsLabel);
+if (isConstructor) {
+cb.opc_aload_1();
+} else {
+cb.opc_aload_2();
+}
 cb.opc_arraylength();
+cb.opc_goto(argCountCheckLabel);
+
+nullArgsLabel.bind();
+cb.emitByte(ClassFileConstants.opc_iconst_0);
+
+argCountCheckLabel.bind();
 cb.opc_sipush((short) parameterTypes.length);
 cb.opc_if_icmpeq(successLabel);
 cb.opc_new(illegalArgumentClass);
diff --git 
a/test/jdk/jdk/internal/reflect/Reflection/MethodAccessGeneratorTest.java 
b/test/jdk/jdk/internal/reflect/Reflection/MethodAccessGeneratorTest.java
new file mode 100644
--- /dev/null
+++ b/test/jdk/jdk/internal/reflect/Reflection/MethodAccessGeneratorTest.java
@@ -0,0 +1,106 @@
+/*
+ * Copyright (c) 2012, 2013, Oracle and/or its affiliates. All rig

[PATCH] JDK-8159797 Throw a right IllegalArgumentException from the bytecode generated by Method/ConstructorAccessor

[Jaikiran Pai]

Hi,

This mail contains a potential patch to the issue[1] that I had reported 
a couple of years back. The issue is still valid and reproducible with 
latest upstream JDK.


In brief, if a certain code has:

public void doSomething(String foo) {...}

and then it gets invoked through reflection:

thatMethod = // get hold of that method through reflection

thatMethod.invoke(validObjectInstance, null);

then as per the javadoc of the Method#invoke[2] you would expect an 
IllegalArgumentException (since the method expects 1 parameter and we 
are sending none). This does throw an IllegalArgumentException, but when 
the invocation happens through a (dynamically generated) MethodAccessor, 
instead of a native MethodAccessor, the IllegalArgumentException that 
gets thrown is due to a NPE that happens and the NPE's toString() output 
is contained as a message of the IllegalArgumentException, as noted in 
the JIRA. This happens even for Constructor invocations, through 
ConstructorAccessor.


The commit in the attached patch, adds bytecode instructions to check if 
the method arguments being passed is null and if so, doesn't attempt an 
arraylength instruction and instead just stores 0 on to the stack. This 
prevents the NullPointerException being reported, enclosed as a message 
within the IllegalArgumentException and instead throws back a proper 
IllegalArgumentException (as you would expect if the invocation had 
happened over a native MethodAccessor).


The goal of the patch _isn't_ to match the exception message with what 
the native MethodAccessor throws but just prevent the NPE from being 
playing a role in the IllegalArgumentException. i.e. this change 
_doesn't_ throw a new IllegalArgumentException("wrong number of arguments").


The patch also contains a new testcase which reproduces this against the 
current upstream and verifies this patch works.


This is the first time I'm fiddling with bytecode generation, so I would 
appreciate some feedback on the changed bytecode and if there's a 
different/better way to do it. Furthermore, although I do have a signed 
and approved OCA, I will need a sponsor for this patch. Anyone willing 
to review and sponsor, please?


[1] https://bugs.openjdk.java.net/browse/JDK-8159797

[2] 
https://docs.oracle.com/javase/10/docs/api/java/lang/reflect/Method.html#invoke(java.lang.Object,java.lang.Object...)


-Jaikiran

# HG changeset patch
# User Jaikiran Pai 
# Date 1525350350 -19800
#  Thu May 03 17:55:50 2018 +0530
# Node ID 2b535248cf02d90f6fea1c89150a277f16954c04
# Parent  7379e6f906aeb6e69ed8eccda9de285e606ab1ff
JDK-8159797 Prevent NPE from the generated Method/ConstructorAccessor when 
invoking with incorrect number of arguments

diff --git 
a/src/java.base/share/classes/jdk/internal/reflect/ClassFileConstants.java 
b/src/java.base/share/classes/jdk/internal/reflect/ClassFileConstants.java
--- a/src/java.base/share/classes/jdk/internal/reflect/ClassFileConstants.java
+++ b/src/java.base/share/classes/jdk/internal/reflect/ClassFileConstants.java
@@ -137,4 +137,7 @@
 
 // Access flags
 public static final short ACC_PUBLIC = (short) 0x0001;
+
+// push int constant to stack
+public static final byte opc_iconst_0   = (byte) 0x03;
 }
diff --git 
a/src/java.base/share/classes/jdk/internal/reflect/MethodAccessorGenerator.java 
b/src/java.base/share/classes/jdk/internal/reflect/MethodAccessorGenerator.java
--- 
a/src/java.base/share/classes/jdk/internal/reflect/MethodAccessorGenerator.java
+++ 
b/src/java.base/share/classes/jdk/internal/reflect/MethodAccessorGenerator.java
@@ -474,7 +474,13 @@
 //   aload_2 | aload_1 (Method | Constructor)
 //   ifnull 
 // aload_2 | aload_1
+// ifnull 
+// aload_2 | aload_1
 // arraylength
+// goto 
+// 
+// iconst_0
+// 
 // sipush 
 // if_icmpeq 
 // new 
@@ -496,7 +502,22 @@
 } else {
 cb.opc_aload_2();
 }
+Label nullArgsLabel = new Label();
+Label argCountCheckLabel = new Label();
+
+cb.opc_ifnull(nullArgsLabel);
+if (isConstructor) {
+cb.opc_aload_1();
+} else {
+cb.opc_aload_2();
+}
 cb.opc_arraylength();
+cb.opc_goto(argCountCheckLabel);
+
+nullArgsLabel.bind();
+cb.emitByte(ClassFileConstants.opc_iconst_0);
+
+argCountCheckLabel.bind();
 cb.opc_sipush((short) parameterTypes.length);
 cb.opc_if_icmpeq(successLabel);
 cb.opc_new(illegalArgumentClass);
diff --git 
a/test/jdk/jdk/internal/reflect/Reflection/MethodAccessGeneratorTest.java 
b/test/jdk/jdk/internal/reflect/Reflection/MethodAccessGeneratorTest.java
new file mode 100644
--- /dev/null
+++ b/test/jdk/jdk/internal/reflect/Reflection/MethodAccessGeneratorTest.java
@@ -0,0 +1,106 @@
+/*
+ * Copyright (c) 2012, 2013, Oracle and/or its affiliates. All rig

Re: [PATCH] JDK-8159797 Throw a right IllegalArgumentException from the bytecode generated by Method/ConstructorAccessor

[Jaikiran Pai]

Any reviews/sponsors please?

-Jaikiran


On 03/05/18 6:24 PM, Jaikiran Pai wrote:

Hi,

This mail contains a potential patch to the issue[1] that I had 
reported a couple of years back. The issue is still valid and 
reproducible with latest upstream JDK.


In brief, if a certain code has:

public void doSomething(String foo) {...}

and then it gets invoked through reflection:

thatMethod = // get hold of that method through reflection

thatMethod.invoke(validObjectInstance, null);

then as per the javadoc of the Method#invoke[2] you would expect an 
IllegalArgumentException (since the method expects 1 parameter and we 
are sending none). This does throw an IllegalArgumentException, but 
when the invocation happens through a (dynamically generated) 
MethodAccessor, instead of a native MethodAccessor, the 
IllegalArgumentException that gets thrown is due to a NPE that happens 
and the NPE's toString() output is contained as a message of the 
IllegalArgumentException, as noted in the JIRA. This happens even for 
Constructor invocations, through ConstructorAccessor.


The commit in the attached patch, adds bytecode instructions to check 
if the method arguments being passed is null and if so, doesn't 
attempt an arraylength instruction and instead just stores 0 on to the 
stack. This prevents the NullPointerException being reported, enclosed 
as a message within the IllegalArgumentException and instead throws 
back a proper IllegalArgumentException (as you would expect if the 
invocation had happened over a native MethodAccessor).


The goal of the patch _isn't_ to match the exception message with what 
the native MethodAccessor throws but just prevent the NPE from being 
playing a role in the IllegalArgumentException. i.e. this change 
_doesn't_ throw a new IllegalArgumentException("wrong number of 
arguments").


The patch also contains a new testcase which reproduces this against 
the current upstream and verifies this patch works.


This is the first time I'm fiddling with bytecode generation, so I 
would appreciate some feedback on the changed bytecode and if there's 
a different/better way to do it. Furthermore, although I do have a 
signed and approved OCA, I will need a sponsor for this patch. Anyone 
willing to review and sponsor, please?


[1] https://bugs.openjdk.java.net/browse/JDK-8159797

[2] 
https://docs.oracle.com/javase/10/docs/api/java/lang/reflect/Method.html#invoke(java.lang.Object,java.lang.Object...)


-Jaikiran

Re: RFR: 8201274: Launch Single-File Source-Code Programs

[Jaikiran Pai]

Not a reviewer, but a minor comment:

src/jdk.compiler/share/classes/com/sun/tools/javac/launcher/Main.java

+private Path getFile(String[] args) throws Fault {

+if (!Files.exists(file)) {
+// should not happen when invoked from launcher
+throw new Fault(Errors.FileNotFound(file));
+}


Do you think it would be better to check that the passed source file 
path is indeed a regular file, instead of just checking for existence, 
so that it won't then run into IOException in the readFile method, if 
the passed path happens to a directory?


-Jaikiran


On 05/05/18 3:29 AM, Jonathan Gibbons wrote:
Here's an update to the previously proposed patch for JEP 330: Launch 
Single-File Source-Code Programs.
It includes all review feedback so far. The changes are mostly minor, 
but with the addition of more test cases.


The webrev includes a delta-webrev for those that just want to see 
what has changed since last time.


Full webrev: http://cr.openjdk.java.net/~jjg/8201274/webrev.mq/index.html

    Original webrev: 
http://cr.openjdk.java.net/~jjg/8201274/webrev.mq/webrev.v1/index.html
    Delta webrev: 
http://cr.openjdk.java.net/~jjg/8201274/webrev.mq/webrev.v2/index.html


Note that the work is temporarily blocked by JDK-8202387: javac 
--release 11 not supported.
A fix for that is underway and in review: 
http://mail.openjdk.java.net/pipermail/compiler-dev/2018-May/011868.html
This work has been tested using a workaround for this issue, and will 
be tested again when the real fix is in place.


-- Jon

On 04/12/2018 01:15 PM, Jonathan Gibbons wrote:

Please review an initial implementation for the feature described in
JEP 330: Launch Single-File Source-Code Programs.

The work is described in the JEP and CSR, and falls into various parts:

  * The part to handle the new command-line options is in the native
    Java launcher code.
  * The part to invoke the compiler and subsequently execute the code
    found in the source file is in a new class in the jdk.compiler 
module.

  * There are some minor Makefile changes, to add support for a new
    resource file.

There are no changes to javac itself.

JEP: http://openjdk.java.net/jeps/330
JBS: https://bugs.openjdk.java.net/browse/JDK-8201274
CSR: https://bugs.openjdk.java.net/browse/JDK-8201275
Webrev: http://cr.openjdk.java.net/~jjg/8201274/webrev.00/

-- Jon

Re: RFR: 8201274: Launch Single-File Source-Code Programs

[Jaikiran Pai]

Just gave the patch a try by locally building it. Works great! :)

A minor comment- the java usage/help text shows this:

Usage: java [options]  [args...]
   (to execute a class)
   or  java [options] -jar  [args...]
   (to execute a jar file)
   or  java [options] -m [/] [args...]
   java [options] --module [/] [args...]
   (to execute the main class in a module)
   or  java [options] java source-file [args]

Do you think that last line could instead be:

   or  java [options]  [args]

        (to launch a single-file source-codeprogram)

to be consistent with the rest of the usage text?

-Jaikiran


On 05/05/18 3:29 AM, Jonathan Gibbons wrote:
Here's an update to the previously proposed patch for JEP 330: Launch 
Single-File Source-Code Programs.
It includes all review feedback so far. The changes are mostly minor, 
but with the addition of more test cases.


The webrev includes a delta-webrev for those that just want to see 
what has changed since last time.


Full webrev: http://cr.openjdk.java.net/~jjg/8201274/webrev.mq/index.html

    Original webrev: 
http://cr.openjdk.java.net/~jjg/8201274/webrev.mq/webrev.v1/index.html
    Delta webrev: 
http://cr.openjdk.java.net/~jjg/8201274/webrev.mq/webrev.v2/index.html


Note that the work is temporarily blocked by JDK-8202387: javac 
--release 11 not supported.
A fix for that is underway and in review: 
http://mail.openjdk.java.net/pipermail/compiler-dev/2018-May/011868.html
This work has been tested using a workaround for this issue, and will 
be tested again when the real fix is in place.


-- Jon

On 04/12/2018 01:15 PM, Jonathan Gibbons wrote:

Please review an initial implementation for the feature described in
JEP 330: Launch Single-File Source-Code Programs.

The work is described in the JEP and CSR, and falls into various parts:

  * The part to handle the new command-line options is in the native
    Java launcher code.
  * The part to invoke the compiler and subsequently execute the code
    found in the source file is in a new class in the jdk.compiler 
module.

  * There are some minor Makefile changes, to add support for a new
    resource file.

There are no changes to javac itself.

JEP: http://openjdk.java.net/jeps/330
JBS: https://bugs.openjdk.java.net/browse/JDK-8201274
CSR: https://bugs.openjdk.java.net/browse/JDK-8201275
Webrev: http://cr.openjdk.java.net/~jjg/8201274/webrev.00/

-- Jon

Re: [PATCH] JDK-8159797 Throw a right IllegalArgumentException from the bytecode generated by Method/ConstructorAccessor

[Jaikiran Pai]

Mandy, thank you for reviewing this change. Comments inline.


On 11/05/18 11:10 PM, mandy chung wrote:


With your patch, IAE thrown with empty message is less desirable even 
though it does not provide less information than the current message 
(NPE::toString).   I wonder if we could define an exception message in 
MethodAccessorImpl for this IAE or even better null parameter is 
passed in.


I'm open to adding an exception message (just like what the Native 
method accessor does right now). I'll update the patch and send a 
updated version soon.


Furthermore, this code has been written for a long time and not many 
one is close to it.  It would require someone to study the 
implementation and generated code to review your patch properly.  I'll 
be on vacation next week.  I may be able to help when I return.




Sure. I too am pretty new to this code (and OpenJDK in general) so would 
like more reviews, especially the part where this introduces an 
additional "aload" instruction.


-Jaikiran



Mandy

On 5/10/18 8:21 PM, Jaikiran Pai wrote:

Any reviews/sponsors please?

-Jaikiran


On 03/05/18 6:24 PM, Jaikiran Pai wrote:

Hi,

This mail contains a potential patch to the issue[1] that I had 
reported a couple of years back. The issue is still valid and 
reproducible with latest upstream JDK.


In brief, if a certain code has:

public void doSomething(String foo) {...}

and then it gets invoked through reflection:

thatMethod = // get hold of that method through reflection

thatMethod.invoke(validObjectInstance, null);

then as per the javadoc of the Method#invoke[2] you would expect an 
IllegalArgumentException (since the method expects 1 parameter and 
we are sending none). This does throw an IllegalArgumentException, 
but when the invocation happens through a (dynamically generated) 
MethodAccessor, instead of a native MethodAccessor, the 
IllegalArgumentException that gets thrown is due to a NPE that 
happens and the NPE's toString() output is contained as a message of 
the IllegalArgumentException, as noted in the JIRA. This happens 
even for Constructor invocations, through ConstructorAccessor.


The commit in the attached patch, adds bytecode instructions to 
check if the method arguments being passed is null and if so, 
doesn't attempt an arraylength instruction and instead just stores 0 
on to the stack. This prevents the NullPointerException being 
reported, enclosed as a message within the IllegalArgumentException 
and instead throws back a proper IllegalArgumentException (as you 
would expect if the invocation had happened over a native 
MethodAccessor).


The goal of the patch _isn't_ to match the exception message with 
what the native MethodAccessor throws but just prevent the NPE from 
being playing a role in the IllegalArgumentException. i.e. this 
change _doesn't_ throw a new IllegalArgumentException("wrong number 
of arguments").


The patch also contains a new testcase which reproduces this against 
the current upstream and verifies this patch works.


This is the first time I'm fiddling with bytecode generation, so I 
would appreciate some feedback on the changed bytecode and if 
there's a different/better way to do it. Furthermore, although I do 
have a signed and approved OCA, I will need a sponsor for this 
patch. Anyone willing to review and sponsor, please?


[1] https://bugs.openjdk.java.net/browse/JDK-8159797

[2] 
https://docs.oracle.com/javase/10/docs/api/java/lang/reflect/Method.html#invoke(java.lang.Object,java.lang.Object...)


-Jaikiran

Re: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives

[Jaikiran Pai]

Hi Matthias,

I am not a reviewer and neither do I have enough knowledge about whether 
jar/file _names_ are considered security sensitive. However, the patch 
that's proposed for this change, prints the file _path_ (and not just 
the name). That I believe is security sensitive.


-Jaikiran
On 06/07/18 6:14 PM, Baesken, Matthias wrote:
Hi Alan ,so it looks like JDK-8204233 added a switch (system property) 
to enable the enhanced socket IOException messages . That would be an 
option as well for 8205525 . 8205525 adds the jar file name and the 
line number info to the exception message . In case that only the jar 
file name would be considered sensitive , I would prefer to just 
output the line number (and omit the system property ). What do you 
think ? Best regards, Matthias
-Original Message- From: Alan Bateman 
[mailto:alan.bate...@oracle.com] Sent: Montag, 25. Juni 2018 16:52 
To: Baesken, Matthias ; core-libs- 
d...@openjdk.java.net Cc: Lindenmaier, Goetz 
 Subject: Re: [RFR] 8205525 : Improve 
exception messages during manifest parsing of jar archives On 
25/06/2018 15:29, Baesken, Matthias wrote:

Hi, do you consider both the file name and line number as sensitive ?
There was a similar discussion on net-dev recently related to 
leaking host names in exceptions. Something similar may be needed here 
Do you know the outcome of this discussion ? 

All the details are in JDK-8204233 and the associated CSR. -Alan

Re: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives

[Jaikiran Pai]

I agree with Chris. Unlike the connection failure error messages where
the IP/port being part of the error message did add value, I don't think
including the "path" to the jar (something like
/opt/private/foo/bar/helloworld.jar) in case of MANIFEST parsing errors
is really necessary. I think, just the filename (helloworld.jar) and the
line number should be good enough in such failures. I have been trying
to think in what cases the entire path would be necessary, to understand
what's causing the parsing failure and address the issue. So far, I
haven't been able to come up with such a use case.

I do understand, as Alan already noted in this thread, the ZipFile APIs
accept a file "path". So, directly using those paths in the error
messages won't work out. Perhaps this changeset can be changed to do the
necessary work to just get hold of the file name (while constructing the
error message)? That way it wouldn't need any of these security
configurations?

-Jaikiran

On 19/07/18 3:37 PM, Chris Hegarty wrote:
>> On 19 Jul 2018, at 09:07, Baesken, Matthias
>>  wrote: Hello, in the meantime I prepared a
>> CSR : https://bugs.openjdk.java.net/browse/JDK-8207768 
> This CSR seems a little premature. Matthias said: "However so far it
> is still a bit unclear how many exceptions we would like to enhance ,
> so this has to be checked first “. Has this been checked? I have seen
> no update on this or any other email thread. JDK-8204233 [1] was a
> pragmatic and targeted solution to a long standing complaint. It was
> not intended, as is obvious by the newly added property name, to set
> precedent for doing similar all over the platform. If, after a broader
> discussion, this approach is to be applied to several different areas
> of the platform, then maybe JDK-8204233 should be revisited ( noting
> that it is JDK 11, which is currently in RDP 1 ). I think we should
> try to avoid a whole plethora of these properties. -Chris. [1]
> https://bugs.openjdk.java.net/browse/JDK-8204233
>>> jdk.includeInExceptions expands the scope. That might be okay but we
>>> will need to re-visit jdk.net.includeInExceptions and also move the
>>> support to somewhere in jdk.internal so that it can be used by other
>>> code in java.base. 
>> Is there some support code for " jdk.net.includeInExceptions " or do
>> you just mean the name of the property ? Best regards, Matthias
>>> -Original Message- From: Alan Bateman
>>> [mailto:alan.bate...@oracle.com] Sent: Mittwoch, 18. Juli 2018 19:44
>>> To: Baesken, Matthias ; core-libs-
>>> d...@openjdk.java.net; Lindenmaier, Goetz 
>>> Subject: Re: [RFR] 8205525 : Improve exception messages during
>>> manifest parsing of jar archives On 18/07/2018 09:21, Baesken,
>>> Matthias wrote:
 Hi Alan, I'll prepare a CSR . I selected a more general name
 "jdk.includeInExceptions" , because 
>>> there is the idea to enhance more exceptions with additional output .
 In such a case " jdk.util.jar.includeInExceptions" would not really
 help . However so far it is still a bit unclear how many exceptions
 we would like to 
>>> enhance , so this has to be checked first .
>>> jdk.includeInExceptions expands the scope. That might be okay but we
>>> will need to re-visit jdk.net.includeInExceptions and also move the
>>> support to somewhere in jdk.internal so that it can be used by other
>>> code in java.base. -Alan 
>

Arrays.asList returned List - Its modifying/write-through semantics

[Jaikiran Pai]

Consider the following code:

import java.util.Arrays;
import java.util.List;

public class ArraysAsListTest {

    public static void main(final String[] args) throws Exception {
        final List someList = Arrays.asList(new String[] {"a"});
        System.out.println("Removed? " + someList.remove("a"));
    }
}

It uses Arrays.asList to create a java.util.List and then calls a
list.remove(...) on it. This currently runs intothe following exception:

Exception in thread "main" java.lang.UnsupportedOperationException: remove
    at java.base/java.util.Iterator.remove(Iterator.java:102)
    at
java.base/java.util.AbstractCollection.remove(AbstractCollection.java:299)
    at ArraysAsListTest.main(ArraysAsListTest.java:8)

The javadoc of Arrays.asList[1] states the following:

"Returns a fixed-size list backed by the specified array. (Changes to
the returned list "write through" to the array.)..."

and has no other mention of how it's supposed to behave with "write"
operations. Given that the javadoc states that the returned list is
"write through", would that mean a "add/removal" is allowed? Probably
not, because it does say it's a fixed size list.So the size altering,
write operations (like add(), remove()) aren't allowed, but that isn't
clear from the javadoc. So should the javadoc be updated to clarify the
semantics of what changes are "write through" and what changes are not
supported?

Furthermore, the UnsupportedOperationException isn't always thrown from
such a returned list. Consider this minor modification to the above code:

        final List someList = Arrays.asList(new String[] {"a"});
        System.out.println("Removed? " + someList.remove("b"));

I add "a" and remove "b". This now runs fine without exceptions and
prints "Removed? false". Should the implementation just throw the
UnsupportedOperationException irrespective of whether or not the element
is contained insuch a returned list? If not, should that be made clear
in the javadoc (maybe same for addoperations)?

[1]
https://docs.oracle.com/javase/10/docs/api/java/util/Arrays.html#asList(T...)

-Jaikiran

Re: Arrays.asList returned List - Its modifying/write-through semantics

[Jaikiran Pai]

Hi Ivan,


On 15/08/18 1:20 PM, Ivan Gerasimov wrote:
> Hi Jaikiran!
>
> The first part (the documentation clarification) was requested some
> time ago [1], so it may be eventually fixed.
>
> [1] https://bugs.openjdk.java.net/browse/JDK-7033681
Thank you for pointing to that one. I hadn't found it during my search
before sending this mail.

>
> With respect to the second part (throwing UOE from remove(Object)), I
> agree with you that it would be more consistent to always throw it,
> regardless of the presence of the searched item.
> However, it would introduce a change of behavior, so I think it's
> unlikely to happen.
I understand.
>
> It may make sense to update the documentation for all the
> structurally-modifying methods of Collection/List (remove(Object),
> retainAll, removeAll, clear) in the same way it was done for
> List.removeIf() here [2].
>
> [2] https://bugs.openjdk.java.net/browse/JDK-8023339

I looked at that commit and yes, a similar @throws explaining the
UnsupportedOperationException will be helpful.

-Jai
>
> With kind regards,
> Ivan
>
>
> On 8/14/18 10:58 PM, Jaikiran Pai wrote:
>> Consider the following code:
>>
>> import java.util.Arrays;
>> import java.util.List;
>>
>> public class ArraysAsListTest {
>>
>>  public static void main(final String[] args) throws Exception {
>>  final List someList = Arrays.asList(new String[]
>> {"a"});
>>  System.out.println("Removed? " + someList.remove("a"));
>>  }
>> }
>>
>> It uses Arrays.asList to create a java.util.List and then calls a
>> list.remove(...) on it. This currently runs intothe following exception:
>>
>> Exception in thread "main" java.lang.UnsupportedOperationException:
>> remove
>>  at java.base/java.util.Iterator.remove(Iterator.java:102)
>>  at
>> java.base/java.util.AbstractCollection.remove(AbstractCollection.java:299)
>>
>>  at ArraysAsListTest.main(ArraysAsListTest.java:8)
>>
>> The javadoc of Arrays.asList[1] states the following:
>>
>> "Returns a fixed-size list backed by the specified array. (Changes to
>> the returned list "write through" to the array.)..."
>>
>> and has no other mention of how it's supposed to behave with "write"
>> operations. Given that the javadoc states that the returned list is
>> "write through", would that mean a "add/removal" is allowed? Probably
>> not, because it does say it's a fixed size list.So the size altering,
>> write operations (like add(), remove()) aren't allowed, but that isn't
>> clear from the javadoc. So should the javadoc be updated to clarify the
>> semantics of what changes are "write through" and what changes are not
>> supported?
>>
>> Furthermore, the UnsupportedOperationException isn't always thrown from
>> such a returned list. Consider this minor modification to the above
>> code:
>>
>>  final List someList = Arrays.asList(new String[]
>> {"a"});
>>  System.out.println("Removed? " + someList.remove("b"));
>>
>> I add "a" and remove "b". This now runs fine without exceptions and
>> prints "Removed? false". Should the implementation just throw the
>> UnsupportedOperationException irrespective of whether or not the element
>> is contained insuch a returned list? If not, should that be made clear
>> in the javadoc (maybe same for addoperations)?
>>
>> [1]
>> https://docs.oracle.com/javase/10/docs/api/java/util/Arrays.html#asList(T...)
>>
>>
>> -Jaikiran
>>
>>
>>
>

Re: Arrays.asList returned List - Its modifying/write-through semantics

[Jaikiran Pai]

Hi Ivan,

On 15/08/18 4:27 PM, Jaikiran Pai wrote:
> Hi Ivan,
>
>
> On 15/08/18 1:20 PM, Ivan Gerasimov wrote:
>> Hi Jaikiran!
>>
>> The first part (the documentation clarification) was requested some
>> time ago [1], so it may be eventually fixed.
>>
>> [1] https://bugs.openjdk.java.net/browse/JDK-7033681
> Thank you for pointing to that one. I hadn't found it during my search
> before sending this mail.

Is this JIRA/change, something that I can contribute? If so, what would
be the typical process for that? I have signed and approved OCA, but I
don't know what's involved in submitting changes which involve javadoc
changes - does this need CSR and will that CSR have to be filed after I
submit the initial draft of the changed javadoc?

-Jaikiran

Re: Arrays.asList returned List - Its modifying/write-through semantics

[Jaikiran Pai]

Thank you Roger and Stuart for the detailed reply. Comments inline.


On 17/08/18 7:27 AM, Stuart Marks wrote:
>  
>
> Note that while this is changing javadoc and doc comments the task is
> quite a bit more rigorous than merely updating documentation. The
> javadoc for public Java SE APIs is a *specification* and requires very
> precise language. It's not unusual for what are apparently small
> changes to go through several rounds of review. (Sorry if you already
> know this, but I wanted to make sure.)
I understand :) I decided to pick this up since it's been around for a
few years now and I don't think anyone is in a hurry to have this done.
So this will give me enough time to understand the contribution process
as well as go through as many review rounds as necessary to get it right.

>
> Whether this requires a CSR depends on whether any normative text of
> the specification is changed. A simple clarification ("API note") can
> be added without a CSR. As the wording stands now, however, it seems
> like there is a mixture of normative and informative statements in the
> text; these should be teased apart and the informative statements
> placed into an API note. In addition, the normative text could
> probably be reworded to be more clear. (See my comments in the bug.)
> Such changes would probably need a CSR, even though they wouldn't
> actually change the intent of the specification.
>
> If you're still with me, then dive right in! It'll probably be easiest
> to exchange drafts on the email list. 
I'll come up with an initial version of the change (based on your inputs
that I see in the JIRA) and send it out to this mailing list for review,
in the coming days.

Thanks again for the detailed reply.

-Jaikiran

[PATCH] JDK-7033681 - Improve the documentation of Arrays.asList

[Jaikiran Pai]

Hello everyone,

I'm requesting a review of a documentation change which was discussed in
a recent thread[1][2]. Here's an initial proposed draft, for a better
documentation of Arrays.asList method:

    /**
 * Returns a fixed-size list backed by the specified array. The passed
 * array is held as a reference in the returned list. Any subsequent
 * changes made to the array contents will be visible in the returned
 * list. Similarly any changes that happen in the returned list will
 * also be visible in the array. The returned list is serializable and
 * implements {@link RandomAccess}.
 *
 * The returned list can be changed only in certain ways. Operations
 * like {@code add}, {@code remove}, {@code clear} and other such, that
 * change the size of the list aren't allowed. Operations like
 * {@code replaceAll}, {@code set}, that change the elements in the list
 * are permitted.
 *
 * This method acts as bridge between array-based and
collection-based
 * APIs, in combination with {@link Collection#toArray}.
 *
 * @apiNote
 * This method also provides a convenient way to create a fixed-size
 * list initialized to contain several elements:
 * 
 * List stooges = Arrays.asList("Larry", "Moe",
"Curly");
 * 
 *
 * The returned list throws a {@link
UnsupportedOperationException} for
 * operations that aren't permitted. Certain implementations of the
returned
 * list might choose to throw the exception only if the call to such
methods
 * results in an actual change, whereas certain other
implementations may
 * always throw the exception when such methods are called.
 *
 * @param  the class of the objects in the array
 * @param a the array by which the list will be backed
 * @return a list view of the specified array
 */
    @SafeVarargs
    @SuppressWarnings("varargs")
    public static  List asList(T... a)


I've edited some of the existing documentation of that method, moved
some existing parts into @apiNote and added additional parts both to the
spec as well as the @apiNote. For a complete reference of what's
changed, I've also attached a patch of this change.

P.S: Is there a specific (make) target that I can run to make sure
changes like this one haven't caused any javadoc generation issues? I
typically run just "make" and did it this time too, but I'm not sure it
parses and generates the javadocs (couldn't find it in the generated
exploded build image).

[1]
http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-August/054894.html

[2] https://bugs.openjdk.java.net/browse/JDK-7033681

-Jaikiran
diff -r cdffba164671 src/java.base/share/classes/java/util/Arrays.java
--- a/src/java.base/share/classes/java/util/Arrays.java Mon Aug 20 10:04:00 
2018 +0200
+++ b/src/java.base/share/classes/java/util/Arrays.java Mon Aug 20 17:53:21 
2018 +0530
@@ -4288,18 +4288,35 @@
 // Misc
 
 /**
- * Returns a fixed-size list backed by the specified array.  (Changes to
- * the returned list "write through" to the array.)  This method acts
- * as bridge between array-based and collection-based APIs, in
- * combination with {@link Collection#toArray}.  The returned list is
- * serializable and implements {@link RandomAccess}.
- *
- * This method also provides a convenient way to create a fixed-size
+ * Returns a fixed-size list backed by the specified array. The passed
+ * array is held as a reference in the returned list. Any subsequent
+ * changes made to the array contents will be visible in the returned
+ * list. Similarly any changes that happen in the returned list will
+ * also be visible in the array. The returned list is serializable and
+ * implements {@link RandomAccess}.
+ *
+ * The returned list can be changed only in certain ways. Operations
+ * like {@code add}, {@code remove}, {@code clear} and other such, that
+ * change the size of the list aren't allowed. Operations like
+ * {@code replaceAll}, {@code set}, that change the elements in the list
+ * are permitted.
+ *
+ * This method acts as bridge between array-based and collection-based
+ * APIs, in combination with {@link Collection#toArray}.
+ *
+ * @apiNote
+ * This method also provides a convenient way to create a fixed-size
  * list initialized to contain several elements:
  * 
  * List stooges = Arrays.asList("Larry", "Moe", "Curly");
  * 
  *
+ * The returned list throws a {@link UnsupportedOperationException} for
+ * operations that aren't permitted. Certain implementations of the 
returned
+ * list might choose to throw the exception only if the call to such 
methods
+ * results in an actual change, whereas certain other implementations may
+ * always throw the exception when such methods are called.
+ *
  * @param  the class of the objects in 

Re: [PATCH] JDK-7033681 - Improve the documentation of Arrays.asList

[Jaikiran Pai]

Anyone willing to help review the patch, please?

-Jaikiran


On 20/08/18 5:56 PM, Jaikiran Pai wrote:
> Hello everyone,
>
> I'm requesting a review of a documentation change which was discussed in
> a recent thread[1][2]. Here's an initial proposed draft, for a better
> documentation of Arrays.asList method:
>
>     /**
>  * Returns a fixed-size list backed by the specified array. The passed
>  * array is held as a reference in the returned list. Any subsequent
>  * changes made to the array contents will be visible in the returned
>  * list. Similarly any changes that happen in the returned list will
>  * also be visible in the array. The returned list is serializable and
>  * implements {@link RandomAccess}.
>  *
>  * The returned list can be changed only in certain ways. Operations
>  * like {@code add}, {@code remove}, {@code clear} and other such, that
>  * change the size of the list aren't allowed. Operations like
>  * {@code replaceAll}, {@code set}, that change the elements in the list
>  * are permitted.
>  *
>  * This method acts as bridge between array-based and
> collection-based
>  * APIs, in combination with {@link Collection#toArray}.
>  *
>  * @apiNote
>  * This method also provides a convenient way to create a fixed-size
>  * list initialized to contain several elements:
>  * 
>  * List<String> stooges = Arrays.asList("Larry", "Moe",
> "Curly");
>  * 
>  *
>  * The returned list throws a {@link
> UnsupportedOperationException} for
>  * operations that aren't permitted. Certain implementations of the
> returned
>  * list might choose to throw the exception only if the call to such
> methods
>  * results in an actual change, whereas certain other
> implementations may
>  * always throw the exception when such methods are called.
>  *
>  * @param  the class of the objects in the array
>  * @param a the array by which the list will be backed
>  * @return a list view of the specified array
>  */
>     @SafeVarargs
>     @SuppressWarnings("varargs")
>     public static  List asList(T... a)
>
>
> I've edited some of the existing documentation of that method, moved
> some existing parts into @apiNote and added additional parts both to the
> spec as well as the @apiNote. For a complete reference of what's
> changed, I've also attached a patch of this change.
>
> P.S: Is there a specific (make) target that I can run to make sure
> changes like this one haven't caused any javadoc generation issues? I
> typically run just "make" and did it this time too, but I'm not sure it
> parses and generates the javadocs (couldn't find it in the generated
> exploded build image).
>
> [1]
> http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-August/054894.html
>
> [2] https://bugs.openjdk.java.net/browse/JDK-7033681
>
> -Jaikiran

Re: [PATCH] JDK-7033681 - Improve the documentation of Arrays.asList

[Jaikiran Pai]

Hello Bernd,

Thank you for the review and sorry about the delayed response. Comments
inline.

On 29/08/18 4:26 PM, Bernd Eckenfels wrote:
> Hello,
>
> Not an Reviewer But just wanted to give a short Feedback: I like the
> new Version it is really helpful. 
>
> However I wonder if the usage example should be outside of the apinote.

+ * @apiNote
+ * This method also provides a convenient way to create a fixed-size
  * list initialized to contain several elements:
  * 
  * List stooges = Arrays.asList("Larry", "Moe", "Curly");
  * 
  *

My limited understanding of the @apiNote is that it is supposed to be
used for additional details of the API and/or its usage examples and the
javadoc itself (outside of the @apiNote) should be a specification of
the API. In this case, I moved that section to @apiNote since that part
appears to mention how/when to use that API in. Having said that, I can
move it out of @apiNote and let it stay the way it previously was, if
you and others feel that's the way to go.

>
> Given the existence of List.of() I wonder if you either mention it as
> a alternative to the example (with slightly different semantic) or
> just remove the sample completely?

I'm not too sure mentioning List.of() construct here will be useful, but
I do see why you mention that. I think the existing example does seem
like a useful usage example, irrespective of whether or not we decide to
have it in or outside of an @apiNote.

-Jaikiran

Re: [PATCH] JDK-7033681 - Improve the documentation of Arrays.asList

[Jaikiran Pai]

On 06/09/18 1:24 PM, Bernd Eckenfels wrote:
> Yes you are right @apinote is aproperiate section (was confusing it with 
> implnote).
>
> Still think a ‚literal specified list‘ is no longer a good (as in canonical) 
> usecase for that method.
>
> I used it in the past often to get a List for using toString() on it, but I 
> guess even for that case List.of can be used instead now.
>
> So @see List#of and let the reader figure out when to use them?

That sounds good to me. I've now updated it to reflect this change and
the javadoc now looks as below. I've also attached the new updated patch.

    /**
 * Returns a fixed-size list backed by the specified array. The passed
 * array is held as a reference in the returned list. Any subsequent
 * changes made to the array contents will be visible in the returned
 * list. Similarly any changes that happen in the returned list will
 * also be visible in the array. The returned list is serializable and
 * implements {@link RandomAccess}.
 *
 * The returned list can be changed only in certain ways. Operations
 * like {@code add}, {@code remove}, {@code clear} and other such, that
 * change the size of the list aren't allowed. Operations like
 * {@code replaceAll}, {@code set}, that change the elements in the list
 * are permitted.
 *
 * This method acts as bridge between array-based and
collection-based
 * APIs, in combination with {@link Collection#toArray}.
 *
 * @apiNote
 * The returned list throws a {@link UnsupportedOperationException} for
 * operations that aren't permitted. Certain implementations of the
returned
 * list might choose to throw the exception only if the call to such
methods
 * results in an actual change, whereas certain other
implementations may
 * always throw the exception when such methods are called.
 *
 * @param  the class of the objects in the array
 * @param a the array by which the list will be backed
 * @return a list view of the specified array
 * @see List#of()
 */



-Jaikiran

# HG changeset patch
# User Jaikiran Pai 
# Date 1535208403 -19800
#  Sat Aug 25 20:16:43 2018 +0530
# Node ID e4d5e71a20f7c80196f211f62440d3cccb69e8f3
# Parent  a716460217ed180972b568e28cf332e37e07a3ae
JDK-7033681 Improve the javadoc of Arrays#toList()

diff --git a/src/java.base/share/classes/java/util/Arrays.java 
b/src/java.base/share/classes/java/util/Arrays.java
--- a/src/java.base/share/classes/java/util/Arrays.java
+++ b/src/java.base/share/classes/java/util/Arrays.java
@@ -4288,21 +4288,33 @@
 // Misc
 
 /**
- * Returns a fixed-size list backed by the specified array.  (Changes to
- * the returned list "write through" to the array.)  This method acts
- * as bridge between array-based and collection-based APIs, in
- * combination with {@link Collection#toArray}.  The returned list is
- * serializable and implements {@link RandomAccess}.
- *
- * This method also provides a convenient way to create a fixed-size
- * list initialized to contain several elements:
- * 
- * List<String> stooges = Arrays.asList("Larry", "Moe", "Curly");
- * 
+ * Returns a fixed-size list backed by the specified array. The passed
+ * array is held as a reference in the returned list. Any subsequent
+ * changes made to the array contents will be visible in the returned
+ * list. Similarly any changes that happen in the returned list will
+ * also be visible in the array. The returned list is serializable and
+ * implements {@link RandomAccess}.
+ *
+ * The returned list can be changed only in certain ways. Operations
+ * like {@code add}, {@code remove}, {@code clear} and other such, that
+ * change the size of the list aren't allowed. Operations like
+ * {@code replaceAll}, {@code set}, that change the elements in the list
+ * are permitted.
+ *
+ * This method acts as bridge between array-based and collection-based
+ * APIs, in combination with {@link Collection#toArray}.
+ *
+ * @apiNote
+ * The returned list throws a {@link UnsupportedOperationException} for
+ * operations that aren't permitted. Certain implementations of the 
returned
+ * list might choose to throw the exception only if the call to such 
methods
+ * results in an actual change, whereas certain other implementations may
+ * always throw the exception when such methods are called.
  *
  * @param  the class of the objects in the array
  * @param a the array by which the list will be backed
  * @return a list view of the specified array
+ * @see List#of()
  */
 @SafeVarargs
 @SuppressWarnings("varargs")

Re: [PATCH] JDK-7033681 - Improve the documentation of Arrays.asList

[Jaikiran Pai]

Any other reviews? I'm not a committer, so I'll also need someone to
help sponsor this patch.

-Jaikiran


On 06/09/18 7:39 PM, Jaikiran Pai wrote:
> On 06/09/18 1:24 PM, Bernd Eckenfels wrote:
>> Yes you are right @apinote is aproperiate section (was confusing it with 
>> implnote).
>>
>> Still think a ‚literal specified list‘ is no longer a good (as in canonical) 
>> usecase for that method.
>>
>> I used it in the past often to get a List for using toString() on it, but I 
>> guess even for that case List.of can be used instead now.
>>
>> So @see List#of and let the reader figure out when to use them?
> That sounds good to me. I've now updated it to reflect this change and
> the javadoc now looks as below. I've also attached the new updated patch.
>
>     /**
>  * Returns a fixed-size list backed by the specified array. The passed
>  * array is held as a reference in the returned list. Any subsequent
>  * changes made to the array contents will be visible in the returned
>  * list. Similarly any changes that happen in the returned list will
>  * also be visible in the array. The returned list is serializable and
>  * implements {@link RandomAccess}.
>  *
>  * The returned list can be changed only in certain ways. Operations
>  * like {@code add}, {@code remove}, {@code clear} and other such, that
>  * change the size of the list aren't allowed. Operations like
>  * {@code replaceAll}, {@code set}, that change the elements in the list
>  * are permitted.
>  *
>  * This method acts as bridge between array-based and
> collection-based
>  * APIs, in combination with {@link Collection#toArray}.
>  *
>  * @apiNote
>  * The returned list throws a {@link UnsupportedOperationException} for
>  * operations that aren't permitted. Certain implementations of the
> returned
>  * list might choose to throw the exception only if the call to such
> methods
>  * results in an actual change, whereas certain other
> implementations may
>  * always throw the exception when such methods are called.
>  *
>  * @param  the class of the objects in the array
>  * @param a the array by which the list will be backed
>  * @return a list view of the specified array
>  * @see List#of()
>  */
>
>
>
> -Jaikiran
>

RFR: 8260401: StackOverflowError on open WindowsPreferences

[Jaikiran Pai]

Can I please get a review for this change which proposes to fix the issue 
reported in https://bugs.openjdk.java.net/browse/JDK-8260401?

As noted in that issue, when the constructor of 
`java.util.prefs.WindowsPreferences` runs into an error while dealing with the 
Windows registry, it logs a warning message. The message construction calls 
`rootNativeHandle()` (on the same instance of `WindowsPreferences` that's being 
constructed) which then ultimately ends up calling the constructor of 
`WindowsPreferences` which then again runs into the Windows registry error and 
attempts to log a message and this whole cycle repeats indefinitely leading to 
that `StackOverFlowError`. 

Based on my limited knowledge of the code in that constructor and analyzing 
that code a bit, it's my understanding (and also the input provided by the 
reporter of the bug) that the log message should actually be using the 
`rootNativeHandle` parameter that is passed to this constructor instead of 
invoking the `rootNativeHandle()` method. The commit in this PR does this 
change to use the passed `rootNativeHandle` in the log message.

Furthermore, there's another constructor in this class which does a similar 
thing and potentially can run into the same error as this one. I've changed 
that constructor too, to avoid the call to `rootNativeHandle()` method in that 
log message. Reading the log message that's being generated there, it's my 
understanding that this change shouldn't cause any inaccuracy in what's being 
logged and in fact, I think it's now more precise about which handle returned 
the error code.

Finally, this log message creation, in both the constructors, also calls 
`windowsAbsolutePath()` and `byteArrayToString()` methods. The 
`byteArrayToString()` is a static method and a call to it doesn't lead back to 
the constructor again. The `windowsAbsolutePath()` is a instance method and 
although it does use a instance variable `absolutePath`, that instance variable 
is already initialized appropriately by the time this `windowsAbsolutePath()` 
gets called in the log message. Also, the `windowsAbsolutePath()` call doesn't 
lead back to the constructor of the `WindowsPreferences` so it doesn't pose the 
same issue as a call to `rootNativeHandle()`.

Given the nature of this issue, I haven't added any jtreg test for this change.

-

Commit messages:
 - 8260401: StackOverflowError on open WindowsPreferences

Changes: https://git.openjdk.java.net/jdk/pull/2326/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2326&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8260401
  Stats: 3 lines in 1 file changed: 0 ins; 0 del; 3 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2326.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2326/head:pull/2326

PR: https://git.openjdk.java.net/jdk/pull/2326

Re: RFR: 8260401: StackOverflowError on open WindowsPreferences

[Jaikiran Pai]

On Mon, 1 Feb 2021 19:21:23 GMT, Brian Burkhalter  wrote:

>> Can I please get a review for this change which proposes to fix the issue 
>> reported in https://bugs.openjdk.java.net/browse/JDK-8260401?
>> 
>> As noted in that issue, when the constructor of 
>> `java.util.prefs.WindowsPreferences` runs into an error while dealing with 
>> the Windows registry, it logs a warning message. The message construction 
>> calls `rootNativeHandle()` (on the same instance of `WindowsPreferences` 
>> that's being constructed) which then ultimately ends up calling the 
>> constructor of `WindowsPreferences` which then again runs into the Windows 
>> registry error and attempts to log a message and this whole cycle repeats 
>> indefinitely leading to that `StackOverFlowError`. 
>> 
>> Based on my limited knowledge of the code in that constructor and analyzing 
>> that code a bit, it's my understanding (and also the input provided by the 
>> reporter of the bug) that the log message should actually be using the 
>> `rootNativeHandle` parameter that is passed to this constructor instead of 
>> invoking the `rootNativeHandle()` method. The commit in this PR does this 
>> change to use the passed `rootNativeHandle` in the log message.
>> 
>> Furthermore, there's another constructor in this class which does a similar 
>> thing and potentially can run into the same error as this one. I've changed 
>> that constructor too, to avoid the call to `rootNativeHandle()` method in 
>> that log message. Reading the log message that's being generated there, it's 
>> my understanding that this change shouldn't cause any inaccuracy in what's 
>> being logged and in fact, I think it's now more precise about which handle 
>> returned the error code.
>> 
>> Finally, this log message creation, in both the constructors, also calls 
>> `windowsAbsolutePath()` and `byteArrayToString()` methods. The 
>> `byteArrayToString()` is a static method and a call to it doesn't lead back 
>> to the constructor again. The `windowsAbsolutePath()` is a instance method 
>> and although it does use a instance variable `absolutePath`, that instance 
>> variable is already initialized appropriately by the time this 
>> `windowsAbsolutePath()` gets called in the log message. Also, the 
>> `windowsAbsolutePath()` call doesn't lead back to the constructor of the 
>> `WindowsPreferences` so it doesn't pose the same issue as a call to 
>> `rootNativeHandle()`.
>> 
>> Given the nature of this issue, I haven't added any jtreg test for this 
>> change.
>
> The code change looks all right. It would be better if there were a test, but 
> apparently this might depend on the user who runs the test not having 
> registry access rights.

Hello Brian,

Thank you for the review.

> It would be better if there were a test, but apparently this might depend on 
> the user who runs the test not having registry access rights.

That's correct. Looking at the code, it looks to me that this will require very 
specific setup of the Windows system to be able to trigger the error.

> The code change looks all right.

Should I go ahead and integrate this?

-

PR: https://git.openjdk.java.net/jdk/pull/2326

Re: RFR: 8260401: StackOverflowError on open WindowsPreferences

[Jaikiran Pai]

On Tue, 2 Feb 2021 02:25:04 GMT, Jaikiran Pai  wrote:

> > The code change looks all right.
> 
> Should I go ahead and integrate this?

Actually, I didn't notice that this PR wasn't marked as reviewed. I'll wait for 
the review(s) then.

-

PR: https://git.openjdk.java.net/jdk/pull/2326

Re: RFR: 8260401: StackOverflowError on open WindowsPreferences

[Jaikiran Pai]

On Tue, 2 Feb 2021 02:41:10 GMT, Brian Burkhalter  wrote:

>>> > The code change looks all right.
>>> 
>>> Should I go ahead and integrate this?
>> 
>> Actually, I didn't notice that this PR wasn't marked as reviewed. I'll wait 
>> for the review(s) then.
>
> I'd let it sit for a bit in case others want to comment.

Ping. Anymore reviews/suggestions from anyone?

-

PR: https://git.openjdk.java.net/jdk/pull/2326

Re: RFR: 8260401: StackOverflowError on open WindowsPreferences

[Jaikiran Pai]

On Fri, 12 Feb 2021 03:21:04 GMT, Brian Burkhalter  wrote:

> Did you run this through the usual CI tests in all tiers?

Hello Brian,

Do you mean other than the ones that have been automatically run and passed in 
the GitHub actions against this PR? I don't have a Windows box, but if there's 
some specific tests you want me to run locally, please do let me know and I'll 
run them.

-

PR: https://git.openjdk.java.net/jdk/pull/2326

Re: RFR: 8260401: StackOverflowError on open WindowsPreferences

[Jaikiran Pai]

On Fri, 12 Feb 2021 23:40:51 GMT, Brian Burkhalter  wrote:

>> Can I please get a review for this change which proposes to fix the issue 
>> reported in https://bugs.openjdk.java.net/browse/JDK-8260401?
>> 
>> As noted in that issue, when the constructor of 
>> `java.util.prefs.WindowsPreferences` runs into an error while dealing with 
>> the Windows registry, it logs a warning message. The message construction 
>> calls `rootNativeHandle()` (on the same instance of `WindowsPreferences` 
>> that's being constructed) which then ultimately ends up calling the 
>> constructor of `WindowsPreferences` which then again runs into the Windows 
>> registry error and attempts to log a message and this whole cycle repeats 
>> indefinitely leading to that `StackOverFlowError`. 
>> 
>> Based on my limited knowledge of the code in that constructor and analyzing 
>> that code a bit, it's my understanding (and also the input provided by the 
>> reporter of the bug) that the log message should actually be using the 
>> `rootNativeHandle` parameter that is passed to this constructor instead of 
>> invoking the `rootNativeHandle()` method. The commit in this PR does this 
>> change to use the passed `rootNativeHandle` in the log message.
>> 
>> Furthermore, there's another constructor in this class which does a similar 
>> thing and potentially can run into the same error as this one. I've changed 
>> that constructor too, to avoid the call to `rootNativeHandle()` method in 
>> that log message. Reading the log message that's being generated there, it's 
>> my understanding that this change shouldn't cause any inaccuracy in what's 
>> being logged and in fact, I think it's now more precise about which handle 
>> returned the error code.
>> 
>> Finally, this log message creation, in both the constructors, also calls 
>> `windowsAbsolutePath()` and `byteArrayToString()` methods. The 
>> `byteArrayToString()` is a static method and a call to it doesn't lead back 
>> to the constructor again. The `windowsAbsolutePath()` is a instance method 
>> and although it does use a instance variable `absolutePath`, that instance 
>> variable is already initialized appropriately by the time this 
>> `windowsAbsolutePath()` gets called in the log message. Also, the 
>> `windowsAbsolutePath()` call doesn't lead back to the constructor of the 
>> `WindowsPreferences` so it doesn't pose the same issue as a call to 
>> `rootNativeHandle()`.
>> 
>> Given the nature of this issue, I haven't added any jtreg test for this 
>> change.
>
> I think you can go ahead and integrate this now.

Thank you Brian for the review  and help in testing the change.

-

PR: https://git.openjdk.java.net/jdk/pull/2326

Integrated: 8260401: StackOverflowError on open WindowsPreferences

[Jaikiran Pai]

On Sat, 30 Jan 2021 14:35:50 GMT, Jaikiran Pai  wrote:

> Can I please get a review for this change which proposes to fix the issue 
> reported in https://bugs.openjdk.java.net/browse/JDK-8260401?
> 
> As noted in that issue, when the constructor of 
> `java.util.prefs.WindowsPreferences` runs into an error while dealing with 
> the Windows registry, it logs a warning message. The message construction 
> calls `rootNativeHandle()` (on the same instance of `WindowsPreferences` 
> that's being constructed) which then ultimately ends up calling the 
> constructor of `WindowsPreferences` which then again runs into the Windows 
> registry error and attempts to log a message and this whole cycle repeats 
> indefinitely leading to that `StackOverFlowError`. 
> 
> Based on my limited knowledge of the code in that constructor and analyzing 
> that code a bit, it's my understanding (and also the input provided by the 
> reporter of the bug) that the log message should actually be using the 
> `rootNativeHandle` parameter that is passed to this constructor instead of 
> invoking the `rootNativeHandle()` method. The commit in this PR does this 
> change to use the passed `rootNativeHandle` in the log message.
> 
> Furthermore, there's another constructor in this class which does a similar 
> thing and potentially can run into the same error as this one. I've changed 
> that constructor too, to avoid the call to `rootNativeHandle()` method in 
> that log message. Reading the log message that's being generated there, it's 
> my understanding that this change shouldn't cause any inaccuracy in what's 
> being logged and in fact, I think it's now more precise about which handle 
> returned the error code.
> 
> Finally, this log message creation, in both the constructors, also calls 
> `windowsAbsolutePath()` and `byteArrayToString()` methods. The 
> `byteArrayToString()` is a static method and a call to it doesn't lead back 
> to the constructor again. The `windowsAbsolutePath()` is a instance method 
> and although it does use a instance variable `absolutePath`, that instance 
> variable is already initialized appropriately by the time this 
> `windowsAbsolutePath()` gets called in the log message. Also, the 
> `windowsAbsolutePath()` call doesn't lead back to the constructor of the 
> `WindowsPreferences` so it doesn't pose the same issue as a call to 
> `rootNativeHandle()`.
> 
> Given the nature of this issue, I haven't added any jtreg test for this 
> change.

This pull request has now been integrated.

Changeset: 849390a1
Author:Jaikiran Pai 
URL:   https://git.openjdk.java.net/jdk/commit/849390a1
Stats: 3 lines in 1 file changed: 0 ins; 0 del; 3 mod

8260401: StackOverflowError on open WindowsPreferences

Reviewed-by: bpb

-

PR: https://git.openjdk.java.net/jdk/pull/2326

Re: Comment on 8259896: Base64 MIME decoder should allow unrecognised characters within padding

[Jaikiran Pai]

Hello Raffaello,

I have added this comment from you, to that JDK-8259896 issue.

-Jaikiran

On 02/02/21 12:43 am, Raffaello Giulietti wrote:

Hi,

in my opinion, the reporter of [1] is right in requiring that 
extraneous characters be discarded, even inside the padding.


Indeed, the first full paragraph on [2] reads:

"Any characters outside of the base64 alphabet are to be ignored in
base64-encoded data."

where "the base64 alphabet" also includes the padding character '=' 
and "base64-encoded data" extends to padding as well, because padding 
is an essential part of encoding.


The legitimate doubt expressed in comment [3] should thus be solved in 
favor of a bug fix.



My 2 cents
Greetings
Raffaello



[1] https://bugs.openjdk.java.net/browse/JDK-8259896
[2] https://tools.ietf.org/html/rfc2045#page-26
[3] 
https://bugs.openjdk.java.net/browse/JDK-8259896?focusedCommentId=14395485&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14395485

ZipInputStream#readAllBytes should clarify it doesn't read the whole stream?

[Jaikiran Pai]

The javadoc of InputStream#readAllBytes() states[1] that it reads all 
the remaining bytes of the stream. The java.util.zip.ZipInputStream 
doesn't override this method and thus "inherits" this javadoc. The 
implementation of InputStream#readAllBytes() ultimately ends up calling 
ZipInputStream#read()[2], so the implementation correctly reads only 
till the end of the current ZipEntry and not the entire ZipInputStream. 
However, because the javadoc gets inherited, reading any code like the 
following doesn't make it clear that it's only reading till the end of 
the current entry:


zis = ... // ZipInputStream
while ((e = zis.getNextEntry()) != null) {
    String name = e.getName();
    zis.readAllBytes(); // gives an impression that all bytes of the 
stream are read

    ...
}

Should the ZipInputStream override the readAllBytes(), just so as to add 
a very specific javadoc to this method which explains that it reads only 
till the end of current entry and other related semantics? Perhaps the 
same should be done for ZipInputStream#readNBytes(...)?



[1] 
https://docs.oracle.com/en/java/javase/15/docs/api/java.base/java/io/InputStream.html#readAllBytes()
[2] 
https://docs.oracle.com/en/java/javase/15/docs/api/java.base/java/util/zip/ZipInputStream.html#read(byte%5B%5D,int,int)


-Jaikiran

Re: ZipInputStream#readAllBytes should clarify it doesn't read the whole stream?

[Jaikiran Pai]

Thank you Lance and Alan. I do have access to JBS, I'll file one 
tomorrow with the details.


-Jaikiran

On 25/02/21 9:04 pm, Lance Andersen wrote:

Hi Jaikiran,

Yes I believe this makes sense.

It would also require a CSR.

I can login a bug if you do not have access to do so.

Best
Lance

On Feb 24, 2021, at 10:56 PM, Jaikiran Pai <mailto:jai.forums2...@gmail.com>> wrote:


The javadoc of InputStream#readAllBytes() states[1] that it reads all 
the remaining bytes of the stream. The java.util.zip.ZipInputStream 
doesn't override this method and thus "inherits" this javadoc. The 
implementation of InputStream#readAllBytes() ultimately ends up 
calling ZipInputStream#read()[2], so the implementation correctly 
reads only till the end of the current ZipEntry and not the entire 
ZipInputStream. However, because the javadoc gets inherited, reading 
any code like the following doesn't make it clear that it's only 
reading till the end of the current entry:


zis = ... // ZipInputStream
while ((e = zis.getNextEntry()) != null) {
    String name = e.getName();
    zis.readAllBytes(); // gives an impression that all bytes of the 
stream are read

    ...
}

Should the ZipInputStream override the readAllBytes(), just so as to 
add a very specific javadoc to this method which explains that it 
reads only till the end of current entry and other related semantics? 
Perhaps the same should be done for ZipInputStream#readNBytes(...)?



[1] 
https://docs.oracle.com/en/java/javase/15/docs/api/java.base/java/io/InputStream.html#readAllBytes() 
<https://docs.oracle.com/en/java/javase/15/docs/api/java.base/java/io/InputStream.html#readAllBytes()>
[2] 
https://docs.oracle.com/en/java/javase/15/docs/api/java.base/java/util/zip/ZipInputStream.html#read(byte%5B%5D,int,int) 
<https://docs.oracle.com/en/java/javase/15/docs/api/java.base/java/util/zip/ZipInputStream.html#read(byte%5B%5D,int,int)>


-Jaikiran






Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering
1 Network Drive
Burlington, MA 01803
lance.ander...@oracle.com <mailto:lance.ander...@oracle.com>

Re: ZipInputStream#readAllBytes should clarify it doesn't read the whole stream?

[Jaikiran Pai]

I just created https://bugs.openjdk.java.net/browse/JDK-8262435 to track 
this.


-Jaikiran

On 25/02/21 9:05 pm, Jaikiran Pai wrote:


Thank you Lance and Alan. I do have access to JBS, I'll file one 
tomorrow with the details.


-Jaikiran

On 25/02/21 9:04 pm, Lance Andersen wrote:

Hi Jaikiran,

Yes I believe this makes sense.

It would also require a CSR.

I can login a bug if you do not have access to do so.

Best
Lance

On Feb 24, 2021, at 10:56 PM, Jaikiran Pai <mailto:jai.forums2...@gmail.com>> wrote:


The javadoc of InputStream#readAllBytes() states[1] that it reads 
all the remaining bytes of the stream. The 
java.util.zip.ZipInputStream doesn't override this method and thus 
"inherits" this javadoc. The implementation of 
InputStream#readAllBytes() ultimately ends up calling 
ZipInputStream#read()[2], so the implementation correctly reads only 
till the end of the current ZipEntry and not the entire 
ZipInputStream. However, because the javadoc gets inherited, reading 
any code like the following doesn't make it clear that it's only 
reading till the end of the current entry:


zis = ... // ZipInputStream
while ((e = zis.getNextEntry()) != null) {
    String name = e.getName();
    zis.readAllBytes(); // gives an impression that all bytes of the 
stream are read

    ...
}

Should the ZipInputStream override the readAllBytes(), just so as to 
add a very specific javadoc to this method which explains that it 
reads only till the end of current entry and other related 
semantics? Perhaps the same should be done for 
ZipInputStream#readNBytes(...)?



[1] 
https://docs.oracle.com/en/java/javase/15/docs/api/java.base/java/io/InputStream.html#readAllBytes() 
<https://docs.oracle.com/en/java/javase/15/docs/api/java.base/java/io/InputStream.html#readAllBytes()>
[2] 
https://docs.oracle.com/en/java/javase/15/docs/api/java.base/java/util/zip/ZipInputStream.html#read(byte%5B%5D,int,int) 
<https://docs.oracle.com/en/java/javase/15/docs/api/java.base/java/util/zip/ZipInputStream.html#read(byte%5B%5D,int,int)>


-Jaikiran






Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering
1 Network Drive
Burlington, MA 01803
lance.ander...@oracle.com <mailto:lance.ander...@oracle.com>

RFR: 8173970: jar tool should have a way to extract to a directory

[Jaikiran Pai]

Can I please get a review for this patch which proposes to implement the 
enhancement request noted in https://bugs.openjdk.java.net/browse/JDK-8173970?

The commit in this PR introduces the `-o` and `--output-dir` option to the 
`jar` command. The option takes a path to a destination directory as a value 
and extracts the contents of the jar into that directory. This is an optional 
option and the changes in the commit continue to maintain backward 
compatibility where the jar is extracted into current directory, if no `-o` or 
`--output-dir` option has been specified.

As far as I know, there hasn't been any discussion on what the name of this new 
option should be. I was initially thinking of using `-d` but that is currently 
used by the `jar` command for a different purpose. So I decided to use `-o` and 
`--output-dir`. This is of course open for change depending on any suggestions 
in this PR.

The commit in this PR also updates the `jar.properties` file which contains the 
English version of the jar command's `--help` output. However, no changes have 
been done to the internationalization files corresponding to this one (for 
example: `jar_de.properties`), because I don't know what process needs to be 
followed to have those files updated (if at all they need to be updated).

The commit also includes a jtreg testcase which verifies the usage of this new 
option.

-

Commit messages:
 - 8173970: jar tool should have a way to extract to a directory

Changes: https://git.openjdk.java.net/jdk/pull/2752/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2752&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8173970
  Stats: 268 lines in 4 files changed: 262 ins; 0 del; 6 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2752.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2752/head:pull/2752

PR: https://git.openjdk.java.net/jdk/pull/2752

Re: RFR: 8173970: jar tool should have a way to extract to a directory

[Jaikiran Pai]

Hello Lance,

On 27/02/21 1:17 am, Lance Andersen wrote:



p.s. I think it would be useful in the future to start the discussion 
on core-libs-dev prior to creating a PR (or leave it as a draft PR) 
for a feature request.


Thank you for that input, I'll keep that in mind for any similar work in 
future.



-Jaikiran

Re: RFR: 8173970: jar tool should have a way to extract to a directory

[Jaikiran Pai]

On 27/02/21 1:17 am, Lance Andersen wrote:


I believe this would also warrant a CSR to be created as well as 
updates to the jar man page.



I haven't created a CSR before, so I will need some guidance on that 
part. Is it usually created after all the implementation details have 
been decided upon or should it be created now?


-Jaikiran

Re: RFR: 8173970: jar tool should have a way to extract to a directory

[Jaikiran Pai]

Hello Alan,

On 27/02/21 2:23 pm, Alan Bateman wrote:


Yes, the option name will need to be agreed. It would be useful to 
enumerate the options that the other tools are using to specify the 
location where to extract. If you see JBS issues mentioning tar -C not 
supporting chdir when extracting then it might be Solaris tar, which 
isn't the same as GNU tar which has different options. It might be 
better to look at more main stream tools, like unzip although jar -d 
is already taken. It would be nice if there were some consistency with 
other tools in the JDK that doing extracting (The jmod and jimage 
extract commands use use --dir for example).


I had a look at both tar and unzip commands on MacOS and Linux (CentOS) 
setup that I had access to.


--
tar on MacOS:
--

tar --version
bsdtar 3.3.2 - libarchive 3.3.2 zlib/1.2.11 liblzma/5.0.5 bz2lib/1.0.6

The version of this tool has:

-C directory, --cd directory, --directory directory
 In c and r mode, this changes the directory before adding 
the following files.
 In x mode, change directories after opening the archive 
but before extracting

 entries from the archive.

A command like "tar -xzf foo.tar.gz -C /tmp/bar/" works fine and 
extracts the foo.tar.gz from current directory to a target directory 
/tmp/bar/


--
tar on CentOS:
--

tar --version
tar (GNU tar) 1.26

This version of the tool has:

Common options:
   -C, --directory=DIR
  change to directory DIR

Although the wording isn't clear that, when used with -x, it extracts to 
the directory specified in -C, it does indeed behave that way.


Specifically, a command like "tar -xzf foo.tar.gz -C /tmp/bar/" works 
fine and extracts the foo.tar.gz from current directory to a target 
directory /tmp/bar/


---
unzip on both MacOS and CentOS:
---

unzip -v
UnZip 6.00 of 20 April 2009, by Info-ZIP.  Maintained by C. Spieler.

This version of the tool has:

[-d exdir]
  An optional directory to which to extract files.  By 
default, all files and sub-
  directories are recreated in the current directory; the 
-d option allows extrac-
  tion in an arbitrary directory (always assuming one has 
permission to  write  to
  the  directory).  This option need not appear at the end 
of the command line; it
  is also accepted before the zipfile specification (with  
the  normal  options),
  immediately  after  the zipfile specification, or between 
the file(s) and the -x
  option.  The option and directory may be concatenated 
without  any  white  space
  between  them,  but  note  that  this may cause normal 
shell behavior to be sup-
  pressed.  In particular, ``-d ~'' (tilde) is expanded by 
Unix C shells into  the
  name of the user's home directory, but ``-d~'' is treated 
as a literal subdirec-

  tory ``~'' of the current directory.

unzip foo.zip -d /tmp/bar/ works fine and extracts the foo.zip from 
current directory to /tmp/bar/


---
jimage and jmod
---

The jimage and jmod as you note use the --dir option for extracting to 
that specified directory.



Those were the tools I looked at. I think using the -d option with -x 
for the jar command is ruled out since it already is used for a 
different purpose, although for a different "main" operation of the jar 
command.


As for using --dir for this new feature, I don't think it alone will be 
enough. Specifically, I couldn't find a "short form" option for the 
--dir option in the jimage or jmod commands. For the jar extract feature 
that we are discussing here, I think having a short form option (in 
addition to the longer form) is necessary to have it match the usage 
expectations of similar other options that the jar command exposes. So 
even if we do choose --dir as the long form option, we would still need 
a short form for it and since -d is already taken for something else, we 
would still need to come up with a different one. The short form of this 
option could be -C (see below).



I think reusing the -C option, for this new feature, perhaps is a good 
thing. The -C is currently used by the update and create "main" 
operation of the jar command and the man page for this option states:


-C dir
  When creating (c) or updating (u) a JAR file, this option 
temporarily changes
  the directory while processing files specified by the 
file operands. Its
  operation is intended to be similar to the -C option of 
the UNIX tar utility.For
  example, the following command changes to the classes 
directory and adds the

  Bar.class file from that directory to my.jar:

  jar uf my.jar -C classes Bar.class


Using the -C option would indeed align it with the tar command. For