1
\$\begingroup\$

I've been using Guava Closer for non-trivial resource life-time management. It is not thread-safe, and occasionally there is a need for a multi-threaded resource allocation. I could not find a library solution, so I've wrote one myself.

I've done my best to handle exceptions and concurrency and my tests are passing. I'm still not sure about complete correctness and the feel like a simpler solution might be possible.


import java.io.Closeable;
import java.io.IOException;
import java.util.ArrayDeque;
import java.util.Deque;

import com.google.common.base.Throwables;

/**
 * Thread-safe {@link com.google.common.io.Closer}
 * 
 * Closes all registered Closeables strictly in reverse order of registration.
 */
public final class FinalCloser implements Closeable {

    private final Deque<Closeable> stack = new ArrayDeque<>();
    private final Object lock = new Object();
    private boolean closed = false;
    /** Coordinates different closing threads, preventing out-of-order disposal. **/
    private boolean busy = false;
    private Throwable error = null;

    /**
     * Registers the given {@code closeable} to be closed when this
     * {@code FinalCloser} is {@linkplain #close closed}.
     * 
     * @throws IOException is this {@code FinalCloser} is already closed. Given
     *                     {@code closeable} is immediately closed in this case.
     *                     The close operation may be done in another thread.
     * @return the given {@code closeable}
     */
    public <T extends Closeable> T register(T closeable) throws IOException {
        if (closeable == null) // For compatibility with try-with-resources
            return closeable;
        boolean closedCopy;
        synchronized (lock) {
            stack.push(closeable);
            closedCopy = this.closed;
        }
        if (closedCopy) {
            IOException closedException = new ResourceClosedException();
            try {
                close();
            } catch (Throwable e) {
                closedException.addSuppressed(e);
            }
            throw closedException;
        }
        return closeable;
    }

    public boolean isClosed() {
        synchronized (lock) {
            return closed;
        }
    }

    /**
     * Close all registered objects. No futher operations are allowed. May be called
     * twice.
     **/
    @Override
    public void close() throws IOException {
        synchronized (lock) {
            closed = true;
            if (busy) {
                return;
            }
            busy = true;
        }
        try {
            for (;;) {
                Closeable closeable = null;
                synchronized (lock) {
                    closeable = stack.pollFirst();
                    if (closeable == null) {
                        busy = false; // Can't do this in another synchronized section, as another thread may skip
                                        // execution while busy == true
                        break;
                    }
                }
                try {
                    closeable.close();
                } catch (Throwable e) {
                    rememberError(e);
                }
            }
        } finally {
            synchronized (lock) {
                if (error != null) {
                    Throwables.propagateIfPossible(error, IOException.class);
                    throw new AssertionError(error); // not possible, because error can only be either RuntimeException
                                                        // or IOException
                }
            }
        }
    }

    private void rememberError(Throwable e) {
        assert e instanceof RuntimeException || e instanceof IOException || e instanceof Error;
        synchronized (lock) {
            if (error == null) {
                error = e;
            } else {
                error.addSuppressed(e);
            }
        }
    }

}
\$\endgroup\$
4
  • \$\begingroup\$ Closer is designed for use with Java 6 and below. In Java7+, it is recommended to use try-with-resources instead. Are you running code at Java 6 or below? \$\endgroup\$
    – Eric Stein
    Jun 5, 2023 at 1:43
  • \$\begingroup\$ You should explain in comments what you are attempting to achieve with the locking strategy and the flags. I think I understand your thinking behind it but it would have saved a bit of time. \$\endgroup\$ Jun 5, 2023 at 11:46
  • \$\begingroup\$ @EricStein Closer is still useful for modern Java to dispose resources at a later point (for. example, when resource aggregate reaches end of life), when count and type of resources is only known at runtime and to implement RAII. \$\endgroup\$
    – Basilevs
    Jun 5, 2023 at 12:03
  • \$\begingroup\$ @TorbenPutkonen the synchronization is done to make the class thread safe. Flags control order of disposal (the order is important for interdependent resources). There are comments for both synchronization and flags in source code. What exactly can be improved? \$\endgroup\$
    – Basilevs
    Jun 5, 2023 at 12:06

1 Answer 1

1
\$\begingroup\$

As far as I can tell by reading the code, the synchronization should work. The rest is about the coding style.

Please use the default JavaDoc comment format. You got that in the class comment, but the rest use **/ as a closing tag. To me it's a sign that the programmer doesn't care to follow common established conventions and sets up a negative atmosphere.

Use names that describe the purpose of the fields instead of their type.

private final Deque<Closeable> closeables = new ArrayDeque<>();

In a multithreaded context the description of the flags is especially important. The comment for the busy flag applies also to the close flag, as they both coordinate different threads. They should be commented with something along the lines:

/**
 * Signals that the close() method has been called.
 */
private boolean closed = false;

/** 
 * Signals that a thread is executing the polling loop in the close() method.
 * Prevents multiple threads from entering the poll loop.
 */
private boolean busyClosing = false;

/**
 * Error caught when closing a closeable. If multiple errors are
 * caught, the subsequent errors are registered as suppressed errors.
 */
private Throwable error = null;

Do not use end-of-line comments. They're hard to read and maintain and the limited space available makes the comments often unnecessary short. There is no need to try to artificially reduce the number of lines.

// For compatibility with try-with-resources
if (closeable == null) {
    return closeable;
}

The fact that a closeable may be registered while a thread is closing the closer conflicts with the documentation of the close method: Close all registered objects. No futher operations are allowed. May be called twice. Heck, the comment even contradicts itself as says that multiple invocations are allowed. If the register(...) and close() methods followed the contract, there would not be any need for the busy flag. The fact that a closeable will be automatically closed if the closer is closed, should be documented in the API.

I prefer to have labels in infinite loops. It makes it clearer what control structure is affected by the break statement and prevents errors when a nested loops are added. An added bonus is that it documents the purpose of the loop. The for (;;) loop is a buit of a C idiom. I don't think it's worth it to try to save 4 characters.

STACK_POLL: while (true) {
    ... break STACK_POLL;
}

The assertion in rememberError(Throwable) seems fairly useless and just confuses the reader. It's not data that can be provided by the user and the compiler prevents other exception types.

As a humorous note, I can feel what "cgruber" felt when they wrote this: https://github.com/google/guava/blob/master/guava/src/com/google/common/io/Closer.java#L125

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.