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

[Claes Redestad]

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