JDK 10 RFR of 8185092: Data race in FilterOutputStream.close

[Martin Buchholz]

Making close exactly-once as you have done is probably the right strategy.

Probably most of java.io was designed to be "mostly thread-safe" as was
common culture back in the last millennium.  There were no "synchronized"
in FilterOutputStream probably because previously there was no need!

For jdk10 VarHandles are probably generally preferred to j.u.c.atomic
classes.

The lack of thread safety specs (and general java.io love) remains a long
term big problem.

It's weird to add @bug to a test without changing the test itself.  Is the
test really testing this fix?

On Wed, Jul 26, 2017 at 10:52 AM, Brian Burkhalter <
brian.burkhalter at oracle.com> wrote:

> Issue: https://bugs.openjdk.java.net/browse/JDK-8185092
> Patch: see diff below
>
> Change the type of the instance variable “closed” from boolean to
> AtomicBoolean. A similar approach is used in RandomAccessFile [1] and
> AbstractSelector [2]. An alternative would be a volatile boolean and a
> synchronization block [3]. The test is updated only to increment the
> copyright year and add the bug ID.
>
> Thanks,
>
> Brian
>
> [1] http://hg.openjdk.java.net/jdk10/jdk10/jdk/file/
> 26e2e601515e/src/java.base/share/classes/java/io/
> RandomAccessFile.java#l638
> [2] http://hg.openjdk.java.net/jdk10/jdk10/jdk/file/
> 26e2e601515e/src/java.base/share/classes/java/nio/
> channels/spi/AbstractSelector.java#l107
> [3] http://hg.openjdk.java.net/jdk10/jdk10/jdk/file/
> d93f2fd542b7/src/java.base/share/classes/java/io/
> FileOutputStream.java#l348
>
> --- a/src/java.base/share/classes/java/io/FilterOutputStream.java
> +++ b/src/java.base/share/classes/java/io/FilterOutputStream.java
> @@ -25,6 +25,8 @@
>
>  package java.io;
>
> +import java.util.concurrent.atomic.AtomicBoolean;
> +
>  /**
>   * This class is the superclass of all classes that filter output
>   * streams. These streams sit on top of an already existing output
> @@ -50,7 +52,7 @@
>      /**
>       * Whether the stream is closed; implicitly initialized to false.
>       */
> -    private boolean closed;
> +    private AtomicBoolean closed = new AtomicBoolean();
>
>      /**
>       * Creates an output stream filter built on top of the specified
> @@ -162,10 +164,9 @@
>       */
>      @Override
>      public void close() throws IOException {
> -        if (closed) {
> +        if (closed.getAndSet(true)) {
>              return;
>          }
> -        closed = true;
>
>          Throwable flushException = null;
>          try {
>
> +++ b/test/java/io/etc/FailingFlushAndClose.java
> @@ -1,5 +1,5 @@
>  /*
> - * Copyright (c) 2011, Oracle and/or its affiliates. All rights reserved.
> + * Copyright (c) 2011, 2017, Oracle and/or its affiliates. All rights
> reserved.
>   * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
>   *
>   * This code is free software; you can redistribute it and/or modify it
> @@ -25,7 +25,7 @@
>
>  /**
>   * @test
> - * @bug 7015589 8054565
> + * @bug 7015589 8054565 8185092
>   * @summary Test that buffering streams are considered closed even when
> the
>   *    close or flush from the underlying stream fails.
>   */
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/nio-dev/attachments/20170726/9c29061d/attachment.html>