<html>
<head>
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
</head>
<body style="background-color: rgb(255, 255, 255); color: rgb(0, 0,
0);" text="#000000" bgcolor="#FFFFFF">
Hi Brad,<br>
<br>
Here's next webrev which tries to cover all your comments:<br>
<br>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~plevart/jdk9-dev/FileInputStreamPool.8047769/webrev.04/">http://cr.openjdk.java.net/~plevart/jdk9-dev/FileInputStreamPool.8047769/webrev.04/</a><br>
<br>
Answers inline...<br>
<br>
<div class="moz-cite-prefix">On 12/30/2014 02:48 AM, Bradford
Wetmore wrote:<br>
</div>
<blockquote cite="mid:54A20475.3040206@oracle.com" type="cite"><!--[if !IE]><DIV style="border-left: 2px solid #009900; border-right: 2px solid #009900; padding: 0px 15px; margin: 2px 0px;"><![endif]-->I'm
looking at this version of the webrev.
<br>
<br>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~plevart/jdk9-dev/FileInputStreamPool.8047769/webrev.03/">http://cr.openjdk.java.net/~plevart/jdk9-dev/FileInputStreamPool.8047769/webrev.03/</a>
<br>
<br>
I just assigned 8047769 to you. My username is wetmore, Alan is
alanb.
<br>
<!--[if !IE]></DIV><![endif]--></blockquote>
<br>
I'll note you both as reviewers in the changeset.<br>
<br>
<blockquote cite="mid:54A20475.3040206@oracle.com" type="cite"><!--[if !IE]><DIV style="border-left: 2px solid #009900; border-right: 2px solid #009900; padding: 0px 15px; margin: 2px 0px;"><![endif]-->
<br>
On 12/24/2014 3:37 AM, Peter Levart wrote:
<br>
<br>
<blockquote type="cite"><!--[if !IE]><DIV style="border-left: 2px solid #009900; border-right: 2px solid #009900; padding: 0px 15px; margin: 2px 0px;"><![endif]-->
<blockquote type="cite"><!--[if !IE]><DIV style="border-left: 2px solid #009900; border-right: 2px solid #009900; padding: 0px 15px; margin: 2px 0px;"><![endif]-->Looks
like you have a committer status, will you be pushing this?
<br>
<!--[if !IE]></DIV><![endif]--></blockquote>
<br>
I can, yes. As soon as we clear-out the remaining questions,
right?
<br>
<!--[if !IE]></DIV><![endif]--></blockquote>
<br>
Yes. The comments below are minor and shouldn't need another
review cycle. <br>
<!--[if !IE]></DIV><![endif]--></blockquote>
<br>
I'm worried about the failure of the test you observed while running
from NetBeans. Perhaps a 0.5s wait is sometimes not enough for
ReferenceHandler thread to process (enqueue) a WeakReference. Since
there is already a facility in place to help ReferenceHandler thread
instead of wait for it, I used it in new version of the test.<br>
<br>
<br>
<blockquote cite="mid:54A20475.3040206@oracle.com" type="cite"><!--[if !IE]><DIV style="border-left: 2px solid #009900; border-right: 2px solid #009900; padding: 0px 15px; margin: 2px 0px;"><![endif]-->
I have started a JPRT job for you, I'll run it through "core"
target which will give us:
<br>
<br>
jdk_lang, jdk_math, jdk_util, jdk_io, jdk_net, jdk_nio,
jdk_security*, jdk_rmi, jdk_text, jdk_time, jdk_other, core_tools.
<br>
<br>
Anything else? I'm off tomorrow, I should have full results Wed.
<br>
<br>
Here are the preliminary results for the jobs that have finished.
jdk.testlibrary.Asserts is causing compilation errors, additional
comments below:
<br>
<br>
/opt/jprt/T/P1/003505.brwetmor/s/jdk/test/sun/security/provider/FileInputStreamPool/FileInputStreamPoolTest.java:33:
error: package jdk.testlibrary does not exist
<br>
import static jdk.testlibrary.Asserts.*;
<br>
^
<br>
/opt/jprt/T/P1/003505.brwetmor/s/jdk/test/sun/security/provider/FileInputStreamPool/FileInputStreamPoolTest.java:52:
error: cannot find symbol
<br>
assertEquals(bytes.length, nread, "short read");
<br>
^
<br>
symbol: method assertEquals(int,int,String)
<br>
location: class FileInputStreamPoolTest
<br>
/opt/jprt/T/P1/003505.brwetmor/s/jdk/test/sun/security/provider/FileInputStreamPool/FileInputStreamPoolTest.java:53:
error: cannot find symbol
<br>
assertTrue(Arrays.equals(readBytes, bytes),
<br>
^
<br>
symbol: method assertTrue(boolean,String)
<br>
location: class FileInputStreamPoolTest
<br>
3 errors
<br>
<br>
TEST RESULT: Failed. Compilation failed: Compilation failed
<br>
<!--[if !IE]></DIV><![endif]--></blockquote>
<br>
I changed the test to be self-contained now so one can run it
without testlib in classpath.<br>
<br>
<blockquote cite="mid:54A20475.3040206@oracle.com" type="cite"><!--[if !IE]><DIV style="border-left: 2px solid #009900; border-right: 2px solid #009900; padding: 0px 15px; margin: 2px 0px;"><![endif]-->
<br>
I'm also getting failures in the following test. I unfortunately
have to leave now, so don't have time to look at this. But it did
mention "seed" so I'm mentioning it here.
<br>
<br>
TEST: java/lang/invoke/LFCaching/LFGarbageCollectedTest.java
<br>
<br>
ACTION: main -- Failed. Execution failed: `main' threw exception:
java.lang.Error: 36 of 39 test cases FAILED! Rerun the test with
the same "-Dseed=" option as in the log file!
<br>
REASON: User specified action: run main/othervm
LFGarbageCollectedTest
<br>
TIME: 213.406 seconds
<br>
messages:
<br>
command: main LFGarbageCollectedTest
<br>
reason: User specified action: run main/othervm
LFGarbageCollectedTest
<br>
elapsed time (seconds): 213.406
<br>
STDOUT:
<br>
-Dseed=6102311124531075592
<br>
-DtestLimit=2000
<br>
Number of iterations according to -DtestLimit is 153 (1989 cases)
<br>
Code cache size is 251658240 bytes
<br>
Non-profiled code cache size is 123058253 bytes
<br>
Number of iterations limited by code cache size is 84 (1092 cases)
<br>
Number of iterations limited by non-profiled code cache size is 41
(533 cases)
<br>
Number of iterations is set to 41 (533 cases)
<br>
Not enough time to continue execution. Interrupted.
<br>
STDERR:
<br>
Iteration 0:
<br>
Tested LF caching feature with MethodHandles.foldArguments method.
<br>
java.lang.AssertionError: Error: Lambda form is not garbage
collected
<br>
at
LFGarbageCollectedTest.doTest(LFGarbageCollectedTest.java:81)
<br>
at
LambdaFormTestCase$TestRun.doIteration(LambdaFormTestCase.java:139)
<br>
at LambdaFormTestCase$$Lambda$2/5042013.call(Unknown
Source)
<br>
at
jdk.testlibrary.TimeLimitedRunner.call(TimeLimitedRunner.java:71)
<br>
at
LambdaFormTestCase.runTests(LambdaFormTestCase.java:201)
<br>
at
LFGarbageCollectedTest.main(LFGarbageCollectedTest.java:105)
<br>
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native
Method)
<br>
<br>
<blockquote type="cite"><!--[if !IE]><DIV style="border-left: 2px solid #009900; border-right: 2px solid #009900; padding: 0px 15px; margin: 2px 0px;"><![endif]-->
<blockquote type="cite"><!--[if !IE]><DIV style="border-left: 2px solid #009900; border-right: 2px solid #009900; padding: 0px 15px; margin: 2px 0px;"><![endif]-->In
a couple places, there are a few lines > 80 chars. (It's a
pet
<br>
peeve of mine, this makes side-by-side reviews difficult
without a
<br>
wide monitor. I realize not everyone shares this view, but
they're
<br>
probably not working on a laptop screen regularly.)
<br>
<!--[if !IE]></DIV><![endif]--></blockquote>
<br>
I have wrapped the lines to contain them inside the 80 column
margin.
<br>
<!--[if !IE]></DIV><![endif]--></blockquote>
<br>
I and my scrubber/slider thank you. :)
<br>
<br>
Two minor nits? SeedGenerator.java: Lines 507/518
<br>
<!--[if !IE]></DIV><![endif]--></blockquote>
<br>
Done that too.<br>
<br>
<blockquote cite="mid:54A20475.3040206@oracle.com" type="cite"><!--[if !IE]><DIV style="border-left: 2px solid #009900; border-right: 2px solid #009900; padding: 0px 15px; margin: 2px 0px;"><![endif]-->
<br>
<blockquote type="cite"><!--[if !IE]><DIV style="border-left: 2px solid #009900; border-right: 2px solid #009900; padding: 0px 15px; margin: 2px 0px;"><![endif]-->
<blockquote type="cite"><!--[if !IE]><DIV style="border-left: 2px solid #009900; border-right: 2px solid #009900; padding: 0px 15px; margin: 2px 0px;"><![endif]-->Do
you need to close the InputStream when last holder is GC'd, or
do
<br>
we just let the FileInputStream's finalizer take care of that?
<br>
<!--[if !IE]></DIV><![endif]--></blockquote>
<br>
WeakReference<UncloseableInputStream> is enqueued when it
is cleared, so
<br>
at that time we have no access to the referent
(UncloseableInputStream)
<br>
any more. We could, in addition, "strongly" reference the
underlying
<br>
FileInputStream in the WeakReference subclass itself, but that
would
<br>
just prolong the life of FileInputStream (possibly forever if no
further
<br>
calls to FileInputStreamPool are made that expunge the
references from
<br>
the queue). So yes, we rely on FileInputStream's finalizer, but
I don't
<br>
see any other way that would be better than that.
<br>
<!--[if !IE]></DIV><![endif]--></blockquote>
<br>
Makes sense, thanks.
<br>
<br>
<blockquote type="cite"><!--[if !IE]><DIV style="border-left: 2px solid #009900; border-right: 2px solid #009900; padding: 0px 15px; margin: 2px 0px;"><![endif]-->NativePRNG
and
<br>
URLSeedGenerator don't have a means of closing underlying
resources
<br>
either, so this is not making things any worse.
<br>
<!--[if !IE]></DIV><![endif]--></blockquote>
<br>
Yes.
<br>
<br>
<blockquote type="cite"><!--[if !IE]><DIV style="border-left: 2px solid #009900; border-right: 2px solid #009900; padding: 0px 15px; margin: 2px 0px;"><![endif]-->
<blockquote type="cite"><!--[if !IE]><DIV style="border-left: 2px solid #009900; border-right: 2px solid #009900; padding: 0px 15px; margin: 2px 0px;"><![endif]-->Both
of these current calls are contained within a
<br>
AccessContrller.doPriviledged, the checkRead() seems
redundant.
<br>
<!--[if !IE]></DIV><![endif]--></blockquote>
<br>
That's right, but from encapsulation, uniformity, security and
future
<br>
maintainability standpoint, I would keep this logic in. It
doesn't hurt.
<br>
Another possibility is to move doPriviliged call to
FileInputStreamPool
<br>
itself and declare it's API exposing security capability (of
reading any
<br>
file).
<br>
<!--[if !IE]></DIV><![endif]--></blockquote>
<br>
I see this was addressed later via Alan's review.
<br>
<br>
<blockquote type="cite"><!--[if !IE]><DIV style="border-left: 2px solid #009900; border-right: 2px solid #009900; padding: 0px 15px; margin: 2px 0px;"><![endif]-->
<blockquote type="cite"><!--[if !IE]><DIV style="border-left: 2px solid #009900; border-right: 2px solid #009900; padding: 0px 15px; margin: 2px 0px;"><![endif]-->In
your test case, if assertions are not enabled, the tests at
<br>
46/50/51 are noops, e.g. when run by hand. Generally should
directly
<br>
throw Exceptions.
<br>
<!--[if !IE]></DIV><![endif]--></blockquote>
<br>
I modified the test to use jdk.testlibrary.Asserts class. Is
this ok
<br>
from "run by hand" perspective or should the test be
self-contained?
<br>
<!--[if !IE]></DIV><![endif]--></blockquote>
<br>
I've not used this Asserts library yet. Is this part of TestNG,
or something new in jtreg or jprt? If Core-libs is ok with this
style of doing it, I'm ok. I'm kind of old-school and tests
should be mostly self-contained and can be tested via:
<br>
<br>
% javac Clazz.java
<br>
% java Clazz
<br>
<!--[if !IE]></DIV><![endif]--></blockquote>
<br>
This should work now.<br>
<br>
<blockquote cite="mid:54A20475.3040206@oracle.com" type="cite"><!--[if !IE]><DIV style="border-left: 2px solid #009900; border-right: 2px solid #009900; padding: 0px 15px; margin: 2px 0px;"><![endif]-->
<br>
without extra classpaths needed. I understand this model doesn't
work with @library and such, so I'm not strongly tied to it. I
can be taught new tricks.
<br>
<!--[if !IE]></DIV><![endif]--></blockquote>
<blockquote cite="mid:54A20475.3040206@oracle.com" type="cite"><!--[if !IE]><DIV style="border-left: 2px solid #009900; border-right: 2px solid #009900; padding: 0px 15px; margin: 2px 0px;"><![endif]-->
<br>
<br>
<blockquote type="cite"><!--[if !IE]><DIV style="border-left: 2px solid #009900; border-right: 2px solid #009900; padding: 0px 15px; margin: 2px 0px;"><![endif]-->
<blockquote type="cite"><!--[if !IE]><DIV style="border-left: 2px solid #009900; border-right: 2px solid #009900; padding: 0px 15px; margin: 2px 0px;"><![endif]-->Core-libs
folks?
<br>
<!--[if !IE]></DIV><![endif]--></blockquote>
<br>
The documentation doesn't mention threads anywhere in
InputStream or
<br>
FileInputStream except in this piece of javadoc for available()
method:
<br>
<!--[if !IE]></DIV><![endif]--></blockquote>
...snip...
<br>
<br>
Ok.
<br>
<br>
A few minor nits below:
<br>
<br>
FileInputStreamPool.java
<br>
========================
<br>
* This method opens an underlying {@link java.io.FileInputStream}
for
<br>
->
<br>
* This method opens an underlying {@link java.io.FileInputStream}
for a
<br>
<br>
* among multiple readers of same {@code file} and ignores
<br>
->
<br>
* among multiple readers of the same {@code file} and ignores
<br>
<!--[if !IE]></DIV><![endif]--></blockquote>
<br>
Done.<br>
<br>
<blockquote cite="mid:54A20475.3040206@oracle.com" type="cite"><!--[if !IE]><DIV style="border-left: 2px solid #009900; border-right: 2px solid #009900; padding: 0px 15px; margin: 2px 0px;"><![endif]-->
<br>
FileInputStreamPoolTest.java
<br>
============================
<br>
Generally JTREG labels are immediately following the copyright and
before the imports.
<br>
<br>
While what you have is allowed by the JTREG syntax, @test is
usually by itself, or followed by old SCCS or filename info.
<br>
<br>
@summary is usually the bug description. Suggest:
<br>
<br>
@summary SecureRandom should be more frugal with file
descriptors
<br>
<br>
48: This is still using assert.
<br>
<!--[if !IE]></DIV><![endif]--></blockquote>
<br>
Fixed.<br>
<br>
<blockquote cite="mid:54A20475.3040206@oracle.com" type="cite"><!--[if !IE]><DIV style="border-left: 2px solid #009900; border-right: 2px solid #009900; padding: 0px 15px; margin: 2px 0px;"><![endif]-->
<br>
Maybe issue multiple reads to exercise in1 and in2? e.g. 2 bytes
of in1, 4 bytes of in2, then 2 bytes of in1?
<br>
<!--[if !IE]></DIV><![endif]--></blockquote>
<br>
The 1st assert makes sure in1 == in2, so there's no point in
invoking the same instance via two aliases.<br>
<br>
<blockquote cite="mid:54A20475.3040206@oracle.com" type="cite"><!--[if !IE]><DIV style="border-left: 2px solid #009900; border-right: 2px solid #009900; padding: 0px 15px; margin: 2px 0px;"><![endif]-->
<br>
IIRC, when I ran this under NetBeans last week, the second
testCaching didn't clear the WeakReference.
<br>
<!--[if !IE]></DIV><![endif]--></blockquote>
<br>
This should not happen any more now that the test is helping to
enqueue the WeakReferences instead of waiting for ReferenceHandler
to enqueue them. The test can now fail only if System.gc() does not
trigger WeakReference processing in the VM. Can you give it a try on
your NetBeans environment?<br>
<br>
<blockquote cite="mid:54A20475.3040206@oracle.com" type="cite"><!--[if !IE]><DIV style="border-left: 2px solid #009900; border-right: 2px solid #009900; padding: 0px 15px; margin: 2px 0px;"><![endif]-->
<br>
Thanks,
<br>
<br>
Brad
<br>
<!--[if !IE]></DIV><![endif]--></blockquote>
<br>
<br>
Regards, Peter<br>
<br>
</body>
</html>