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