RFR: 8168640: (fc) Avoiding AtomicBoolean in FileInput/-OutputStream improves startup

[Peter Levart]

Hi David,


On 10/27/2016 04:57 AM, David Holmes wrote:
> On 25/10/2016 10:36 PM, Peter Levart wrote:
>> Hi Claes,
>>
>>
>> On 10/25/2016 01:09 PM, Aleksey Shipilev wrote:
>>> On 10/25/2016 12:51 PM, Claes Redestad wrote:
>>>> Avoiding AtomicBoolean improves startup and footprint for a sample of
>>>> small applications:
>>>>
>>>> Webrev: http://cr.openjdk.java.net/~redestad/8168640/webrev.01/
>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8168640
>>> I would generally disagree to avoid Atomics to improve startup. In this
>>> case, we always lock on close(), even for a short time. I wonder if
>>> using plain Unsafe.compareAndSwap instead of Atomic would be cleaner,
>>> instead of introducing performance bugs with synchronized-s.
>>>
>>> Thanks,
>>> -Aleksey
>>>
>>>
>>
>> In addition, there is no need for this:
>>
>>  379                     boolean closed;
>>  380                     synchronized (closeLock) {
>>  381                         closed = this.closed;
>>  382                     }
>>
>> A simple:
>>
>>         boolean closed = this.closed;
>>
>> is equivalent, since this.closed is volatile.
>
> Sorry but where exactly is the code referenced above?

It was in the previous webrev in the patched FileInputStream. But the 
webrev has been changed in-place...

Regards, Peter

>
> Thanks,
> David
>
>>
>> But something else bothers me with this code. There is a race. Here are
>> the relevant parts:
>>
>>  316     public void close() throws IOException {
>>  317         if (closed) {
>>  318             return;
>>  319         }
>>  320         synchronized (closeLock) {
>>  321             if (closed) {
>>  322                 return;
>>  323             }
>>  324             closed = true;
>>  325         }
>>  326
>>  327         FileChannel fc = channel;
>>  328         if (fc != null) {
>>  329            fc.close();
>>  330         }
>>  331
>>  332         fd.closeAll(new Closeable() {
>>  333             public void close() throws IOException {
>>  334                close0();
>>  335            }
>>  336         });
>>  337     }
>>
>> and:
>>
>>  372     public FileChannel getChannel() {
>>  373         FileChannel fc = this.channel;
>>  374         if (fc == null) {
>>  375             synchronized (this) {
>>  376                 fc = this.channel;
>>  377                 if (fc == null) {
>>  378                     this.channel = fc = FileChannelImpl.open(fd,
>> path, true, false, this);
>>  379                     boolean closed;
>>  380                     synchronized (closeLock) {
>>  381                         closed = this.closed;
>>  382                     }
>>  383                     if (closed) {
>>  384                         try {
>>  385                             fc.close();
>>  386                         } catch (IOException ioe) {
>>  387                             throw new InternalError(ioe); // should
>> not happen
>>  388                         }
>>  389                     }
>>  390                 }
>>  391             }
>>  392         }
>>  393         return fc;
>>  394     }
>>
>>
>> Suppose Thread 1 is executing close() up to line 326, Then Thread 2
>> kicks-in and executes getChannel() for the 1st time on this
>> FileInputStream. It opens FileChannelImpl and finds closed flag already
>> set, so it closes the channel. Thread 1 then resumes from line 326 and
>> finds 'channel' field already set, so it closes the channel once more.
>>
>> FileChannelImpl.close() may be idempotent, but why not making sure it is
>> called just once?
>>
>> In order to fix this, you should read the 'channel' field in close()
>> while holding closeLock and you should publish the 'channel' field in
>> getChannel() while holding closeLock. Like this:
>>
>> diff -r 2e7a303cd1ec
>> src/java.base/share/classes/java/io/FileInputStream.java
>> --- a/src/java.base/share/classes/java/io/FileInputStream.java Wed Oct
>> 19 11:45:43 2016 +0800
>> +++ b/src/java.base/share/classes/java/io/FileInputStream.java Tue Oct
>> 25 14:33:05 2016 +0200
>> @@ -26,7 +26,6 @@
>>  package java.io;
>>
>>  import java.nio.channels.FileChannel;
>> -import java.util.concurrent.atomic.AtomicBoolean;
>>  import sun.nio.ch.FileChannelImpl;
>>
>>
>> @@ -60,7 +59,9 @@
>>
>>      private volatile FileChannel channel;
>>
>> -    private final AtomicBoolean closed = new AtomicBoolean(false);
>> +    private final Object closeLock = new Object();
>> +
>> +    private volatile boolean closed;
>>
>>      /**
>>       * Creates a <code>FileInputStream</code> by
>> @@ -313,12 +314,18 @@
>>       * @spec JSR-51
>>       */
>>      public void close() throws IOException {
>> -        if (!closed.compareAndSet(false, true)) {
>> -            // if compareAndSet() returns false closed was already true
>> +        if (closed) {
>>              return;
>>          }
>> +        FileChannel fc;
>> +        synchronized (closeLock) {
>> +            if (closed) {
>> +                return;
>> +            }
>> +            closed = true;
>> +            fc = channel;
>> +        }
>>
>> -        FileChannel fc = channel;
>>          if (fc != null) {
>>             fc.close();
>>          }
>> @@ -369,8 +376,13 @@
>>              synchronized (this) {
>>                  fc = this.channel;
>>                  if (fc == null) {
>> -                    this.channel = fc = FileChannelImpl.open(fd, path,
>> true, false, this);
>> -                    if (closed.get()) {
>> +                    fc = FileChannelImpl.open(fd, path, true, false,
>> this);
>> +                    boolean closed;
>> +                    synchronized (closeLock) {
>> +                        closed = this.closed;
>> +                        this.channel = fc;
>> +                    }
>> +                    if (closed) {
>>                          try {
>>                              fc.close();
>>                          } catch (IOException ioe) {
>>
>>
>> Regards, Peter
>>
>>