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