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

David Holmes david.holmes at oracle.com
Thu Oct 27 02:57:23 UTC 2016


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?

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


More information about the core-libs-dev mailing list