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