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

[David Holmes]

On 27/10/2016 3:46 PM, Peter Levart wrote:
> 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...

Okay - nothing to see then :)

Thanks,
David

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