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.
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 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'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
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
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
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.
Maybe issue multiple reads to exercise in1 and in2? e.g. 2 bytes of
in1, 4 bytes of in2, then 2 bytes of in1?
IIRC, when I ran this under NetBeans last week, the second testCaching
didn't clear the WeakReference.
Thanks,
Brad
