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

[Bradford Wetmore]

Hi Peter,

Looks like you have a committer status, will you be pushing this?

On 12/18/2014 5:23 AM, Peter Levart wrote:
> Hi,
>
> I propose a patch for the following issue:
>
>      https://bugs.openjdk.java.net/browse/JDK-8047769
>
> Here's the webrev:
>
> http://cr.openjdk.java.net/~plevart/jdk9-dev/FileInputStreamPool.8047769/webrev.01/

Looks good.  A few minor comments.

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.)

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?

Both of these current calls are contained within a 
AccessContrller.doPriviledged, the checkRead() seems redundant.

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.

> The patch uses a package-private FileInputStreamPool class that caches
> open FileInputStream(s) so that at most one file descriptor is open for
> a particular file. Reading from shared unbuffered FileInputStream in
> multiple threads should be safe, right?

I would think (hope?) so, but I am not 100% sure.  I tracked it down 
into libjava native code:

io_util.c
=========
     fd = GET_FD(this, fid);
     if (fd == -1) {
         JNU_ThrowIOException(env, "Stream Closed");
         nread = -1;
     } else {
         nread = IO_Read(fd, buf, len);

which is then defined to handleRead, which calls something called 
RESTARTABLE in io_util_md.c.  Assuming I'm looking at the right code.

Core-libs folks?

> sun/security/provider/PolicyFile/GrantAllPermToExtWhenNoPolicy.java:
> Make sure that when no system policy and user policy files exist, the
> built-in default policy will be used, which - amongst other things -
> grants standard extensions the AllPermission.
> sun/security/provider/PolicyParser/ExtDirsChange.java: standard
> extensions path is hard-coded in default system policy file
> sun/security/provider/PolicyParser/PrincipalExpansionError.java: parser
> incorrectly ignores a principal if the principal name expands to nothing.
>
> ...they are unrelated to this patch - seems to be caused by recent
> layout changes for modular runtime images.

Hopefully you saw my previous response.  Repeating:

---begin---
They've been failing for a while:

     https://bugs.openjdk.java.net/browse/JDK-8039280

These tests are all "/manual" so they don't show up in our regular runs 
of JPRT/jtreg, which include -a.

     -a | -automatic | -automagic
                 Any test with /manual will not be run
---end---

Thanks,

Brad