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

[Lance Andersen]

Hi Jaikiran,
> On Jul 10, 2019, at 2:56 AM, Jaikiran Pai  wrote:
> 
> 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().
> 
> 
> 

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
>> 
>>> - 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
> 
>>> 
>>> 
>>> - 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?
> 
> 
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 
> The latest webrev with the above noted changes is available at 
> http://cr.openjdk.java.net/~jpai/webrev/8225763/3/webrev/ 
> 
> -Jaikiran
Best
Lance
 
  

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

Re: 8187898: PrintStream should override FilterOutputStream#write(byte[]) with a method that has no throws clause

[Brian Burkhalter]

Testing revealed the need for some changes in 
test/jdk/java/lang/System/LoggerFinder/internal. Webrev updated in place.

Brian

> On Jul 11, 2019, at 10:44 AM, Brian Burkhalter  
> wrote:
> 
> https://bugs.openjdk.java.net/browse/JDK-8187898 
> 
> http://cr.openjdk.java.net/~bpb/8187898/webrev.00/ 
> 
> 
> Override FilterOutputStream.write(byte[]) not to throw IOException. Including 
> 2d-dev as this creates a source compatibility issue in PSPrinterJob which I 
> fix in this patch.
> 
> A CSR would of course be needed for this.

8187898: PrintStream should override FilterOutputStream#write(byte[]) with a method that has no throws clause

[Brian Burkhalter]

https://bugs.openjdk.java.net/browse/JDK-8187898
http://cr.openjdk.java.net/~bpb/8187898/webrev.00/

Override FilterOutputStream.write(byte[]) not to throw IOException. Including 
2d-dev as this creates a source compatibility issue in PSPrinterJob which I fix 
in this patch.

A CSR would of course be needed for this.

Thanks,

Brian

Re: RFR [14/java.xml] 8178843: A bug in an inner loop in MethodGenerator's getLocals method

[Joe Wang]

Thanks Lance!

-Joe

On 7/10/19 6:38 PM, Lance Andersen wrote:

+1
On Jul 10, 2019, at 7:57 PM, Joe Wang > wrote:


Please review a cleanup that removes unused code. This code has two 
errors, one as indicated in the bug report, another attempting to 
loop through allVarsEverDeclared that was just created (meaning 
size=0). It just ensures again that the code was never used.


JBS: https://bugs.openjdk.java.net/browse/JDK-8178843
webrevs: http://cr.openjdk.java.net/~joehw/jdk14/8178843/webrev/

All tests passed.

Thanks,
Joe




Lance 
Andersen| Principal Member of Technical Staff | +1.781.442.2037

Oracle Java Engineering
1 Network Drive
Burlington, MA 01803
lance.ander...@oracle.com 

Re: RFR: 8227587: Add internal privileged System.loadLibrary

[Claes Redestad]

Hi Roger,

On 2019-07-11 16:10, Roger Riggs wrote:

Hi Claes,

JavaLangAccess.java:
316: Add @param tag


done.



System.java:  2282, 2287
Runtime.loadLibrary0 makes a second check for a security manager.
Is there any potential gain by calling ClassLoader.loadLibrary directly?
None of the internal uses would have a separatorChar.


Most of the gain comes from not having to load one-off PA classes or
lambdas, but of course avoiding the indexOf and a few indirections are
marginally helpful to startup. We should at least assert that the
library names are sane, though.



I expect most of the files need a copyright update.
The script in /make/scripts/update_copyright_year.sh should do the 
work for modified files.


Fixed copyrights and updated in place: 
http://cr.openjdk.java.net/~redestad/8227587/open.00


Thanks!

/Claes

Re: RFR: 8227587: Add internal privileged System.loadLibrary

[Roger Riggs]

Hi Claes,

JavaLangAccess.java:
316: Add @param tag

System.java:  2282, 2287
Runtime.loadLibrary0 makes a second check for a security manager.
Is there any potential gain by calling ClassLoader.loadLibrary directly?
None of the internal uses would have a separatorChar.

I expect most of the files need a copyright update.
The script in /make/scripts/update_copyright_year.sh should do the 
work for modified files.


Roger


On 7/11/19 9:43 AM, Claes Redestad wrote:

Hi,

by adding a method to load libraries with privileges to JavaLangAccess,
we can simplify a slew of places where we are currently implementing
adhoc privileged actions. This is a tiny but measurable gain on a range
of startup tests.

Webrev:  http://cr.openjdk.java.net/~redestad/8227587/open.00
Bug: https://bugs.openjdk.java.net/browse/JDK-8227587
Testing: tier1-3

Thanks!

/Claes

Re: 8193072: File.delete() should remove its path from DeleteOnExitHook.files

[Jason Mehrens]

Would it work to fix this by making DeleteOnExitHook::runHooks deal with 
dependencies?
1. Remove If deleted, or not directory which also takes care of not exists.
2. Sort remaining files by deepest child files/directories first.
3. Run delete again on the list.

Otherwise files need to be processed in reverse order before directories and 
directories need to be processed children first up to the root.

Jason


From: core-libs-dev  on behalf of Ivan 
Gerasimov 
Sent: Wednesday, July 10, 2019 8:51 PM
To: Brian Burkhalter; core-libs-dev
Subject: Re: 8193072: File.delete() should remove its path from 
DeleteOnExitHook.files


On 7/10/19 5:17 PM, Brian Burkhalter wrote:
> I incorporated Peter’s version, adding the security check in 
> cancelDeleteOnExit(), tweaking its verbiage along with that of 
> deleteOnExit(), and modified the test DeleteOnExit to verify the new method. 
> The updated version is here:
>
> http://cr.openjdk.java.net/~bpb/8193072/webrev.03/ 
> 
There is possibility of a race here in a scenario like this:

File dir = new File("dir");
File file = new File("dir/file");

-- thread 1 --
dir.deleteOnExit();
file.deleteOnExit();
///
dir.cancelDeleteOnExit();
  thread 2 intervenes
file.cancelDeleteOnExit();
-- end --

-- thread 2 --
dir.deleteOnExit();
file.deleteOnExit();
-- end --

The result will be that the order of the registered files will change,
and JVM will try to delete dir first (this will fail as it is not empty).

Of course it could be avoided, if cancellation were done in reverse
order, though it's not immediately obvious from the documentation.

With kind regards,
Ivan
> Thanks,
>
> Brian
>
>> On Jul 10, 2019, at 11:17 AM, Brian Burkhalter  
>> wrote:
>>
>> On Jul 10, 2019, at 5:36 AM, Peter Levart  wrote:
>>> There are various interleavings of threads that could cause the file to be 
>>> left undeleted when VM exits.
>>>
>>> To cover concurrent scenarios such as above, the code could use reference 
>>> counting. Like in the following patch:
>>>
>>> http://cr.openjdk.java.net/~plevart/jdk-dev/8193072_File.undoDeleteOnExit/webrev.01/
>>>  
>>> 
>> This looks good to me modulo adding this
>> SecurityManager security = System.getSecurityManager();
>> if (security != null) {
>> security.checkDelete(path);
>> }
>> to cancelDeleteOnExit() as suggested below.
>>
>>> On Jul 10, 2019, at 7:51 AM, Sean Mullan  wrote:
>>>
>>> On 7/9/19 7:40 PM, Brian Burkhalter wrote:
 I don’t know. On the one hand this does not take an action like reading, 
 writing, or deleting, but on the other it could end up causing files to be 
 left lying around after VM termination which were expected to be deleted. 
 I suppose that could be considered to be some sort of security issue.
>>> Yes I think so.
>>>
>>> I would probably just use the same permission 
>>> (FilePermission(file,"delete")). If you have been granted permission to 
>>> delete a file, then you should have permission to cancel that deletion as 
>>> well.
>> That’s a  good idea.

--
With kind regards,
Ivan Gerasimov

RFR: 8227587: Add internal privileged System.loadLibrary

[Claes Redestad]

Hi,

by adding a method to load libraries with privileges to JavaLangAccess,
we can simplify a slew of places where we are currently implementing
adhoc privileged actions. This is a tiny but measurable gain on a range
of startup tests.

Webrev:  http://cr.openjdk.java.net/~redestad/8227587/open.00
Bug: https://bugs.openjdk.java.net/browse/JDK-8227587
Testing: tier1-3

Thanks!

/Claes

Re: 8193072: File.delete() should remove its path from DeleteOnExitHook.files

[Peter Levart]

On 7/11/19 9:47 AM, Peter Levart wrote:


http://cr.openjdk.java.net/~plevart/jdk-dev/8193072_File.undoDeleteOnExit/webrev.02/ 





Another thing to consider (done in above webrev.02) is what to do with 
unbalanced cancelDeleteOnExit(). I think it is better to throw exception 
than to silently ignore it. This way unintentional bugs can be uncovered 
which would otherwise just cause erratic behavior.


Regards, Peter

Re: 8193072: File.delete() should remove its path from DeleteOnExitHook.files

[Peter Levart]

Hi,

On 7/11/19 3:51 AM, Ivan Gerasimov wrote:


On 7/10/19 5:17 PM, Brian Burkhalter wrote:
I incorporated Peter’s version, adding the security check in 
cancelDeleteOnExit(), tweaking its verbiage along with that of 
deleteOnExit(), and modified the test DeleteOnExit to verify the new 
method. The updated version is here:


http://cr.openjdk.java.net/~bpb/8193072/webrev.03/ 


There is possibility of a race here in a scenario like this:

File dir = new File("dir");
File file = new File("dir/file");

-- thread 1 --
dir.deleteOnExit();
file.deleteOnExit();
///
dir.cancelDeleteOnExit();
  thread 2 intervenes
file.cancelDeleteOnExit();
-- end --

-- thread 2 --
dir.deleteOnExit();
file.deleteOnExit();
-- end --

The result will be that the order of the registered files will change, 
and JVM will try to delete dir first (this will fail as it is not empty).


Of course it could be avoided, if cancellation were done in reverse 
order, though it's not immediately obvious from the documentation.


Hm,

LinkedHashMap.computeIfAbsent/computeIfPresent can also honor the so 
called "access order" which always moves the entry to the end of the 
linked list regardless of whether the entry is already present or not. 
So in above scenario and if LinkedHashMap was constructed with 
accessOrder=true, the registration order of paths after each operation 
would be as follows (the order of deletion is the reverse order of the 
presented registration order):


File dir = new File("dir");
File file = new File("dir/file");

-- thread 1 --

dir.deleteOnExit();
[dir]

file.deleteOnExit();
[dir, dir/file]

dir.cancelDeleteOnExit();
[dir/file]

-- thread 2 --

dir.deleteOnExit();
[dir/file, dir]

file.deleteOnExit();
[dir, dir/file]

-- thread 1 --

file.cancelDeleteOnExit();
[dir, dir/file]


But that is just coincidence. There are other interleavings which would 
cause LHM "access order" to reorder paths in undesired way. Perhaps the 
best behavior would be for deleteOnExit() to reorder the file to the end 
of the registration list while cancelDeleteOnExit() to not change the 
order of registered paths. For example, like this:


http://cr.openjdk.java.net/~plevart/jdk-dev/8193072_File.undoDeleteOnExit/webrev.02/

This does however change the order of registration for sequences like 
the following:


file1.deleteOnExit();
file2.deleteOnExit();
file1.deleteOnExit();

Order of deletion now: file2, file1
Order of deletion with this patch: file1, file2

But considering that programs that register multiple files do so 
consistently (always the same set of related files in one go in the same 
order) or register unrelated files in unimportant order, such behavior 
is perhaps acceptable.


What do you think?


Regards, Peter