RFR: JDK-8047769 SecureRandom should be more frugal with file descriptors

Bradford Wetmore bradford.wetmore at oracle.com
Tue Dec 30 01:48:37 UTC 2014

I'm looking at this version of the webrev.


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:

error: package jdk.testlibrary does not exist
import static jdk.testlibrary.Asserts.*;
error: cannot find symbol
         assertEquals(bytes.length, nread, "short read");
   symbol:   method assertEquals(int,int,String)
   location: class FileInputStreamPoolTest
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
command: main LFGarbageCollectedTest
reason: User specified action: run main/othervm LFGarbageCollectedTest
elapsed time (seconds): 213.406
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 
Number of iterations is set to 41 (533 cases)
Not enough time to continue execution. Interrupted.
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$$Lambda$2/5042013.call(Unknown Source)
         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.


>> 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:


A few minor nits below:

* 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

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.



More information about the core-libs-dev mailing list