On 2016-10-25 14:36, 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.
Good point, cumulative diff to both files:
fc = this.channel;
if (fc == null) {
this.channel = fc = FileChannelImpl.open(fd, path,
false, true, this);
- boolean closed;
- synchronized (closeLock) {
- closed = this.closed;
- }
if (closed) {
try {
fc.close();
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?
Hmm, it would seem like a pre-existing issue that was not dealt with
neither before nor after JDK-8025619, no?
And FileChannel inherits AbstractInterruptibleChannel::close() (public
final), which specifies behavior: "If the channel has already been
closed then this method returns immediately." Thus I don't think the
extra ceremony is warranted, won't you agree?
Thanks!
/Claes
