From 13a6d481fb1984ec3082719b89e8ee8527119b6a Mon Sep 17 00:00:00 2001 From: "Peter G. Horvath" Date: Sun, 7 Aug 2016 15:51:20 +0200 Subject: [PATCH] Implement AbstractStatement.closeOnCompletion() #15 re-worked the implementation of #15 --- .../base/AbstractAutoCloseableJdbcObject.java | 193 ++++++++++-------- .../collection/RemoveLastItemCallbackSet.java | 39 ---- .../common/util/collection/SetProxy.java | 93 --------- 3 files changed, 112 insertions(+), 213 deletions(-) delete mode 100644 src/main/java/com/github/dyna4jdbc/internal/common/util/collection/RemoveLastItemCallbackSet.java delete mode 100644 src/main/java/com/github/dyna4jdbc/internal/common/util/collection/SetProxy.java diff --git a/src/main/java/com/github/dyna4jdbc/internal/common/jdbc/base/AbstractAutoCloseableJdbcObject.java b/src/main/java/com/github/dyna4jdbc/internal/common/jdbc/base/AbstractAutoCloseableJdbcObject.java index 328f34b..cf89004 100644 --- a/src/main/java/com/github/dyna4jdbc/internal/common/jdbc/base/AbstractAutoCloseableJdbcObject.java +++ b/src/main/java/com/github/dyna4jdbc/internal/common/jdbc/base/AbstractAutoCloseableJdbcObject.java @@ -18,15 +18,12 @@ import java.sql.SQLException; import java.util.Collection; -import java.util.Collections; import java.util.HashSet; import java.util.LinkedList; import java.util.Set; import java.util.concurrent.atomic.AtomicBoolean; import com.github.dyna4jdbc.internal.JDBCError; -import com.github.dyna4jdbc.internal.RuntimeDyna4JdbcException; -import com.github.dyna4jdbc.internal.common.util.collection.RemoveLastItemCallbackSet; /** * {@code AbstractAutoCloseableJdbcObject} is a base class @@ -44,10 +41,11 @@ public class AbstractAutoCloseableJdbcObject extends AbstractWrapper implements AutoCloseable { /** + *

* A thread-safe boolean container for the closed flag, used to prevent further use * of a concrete JDBC class implementation once {@link #close()} is called. * We use {@code AtomicBoolean} here so that a subclass can safely be closed from a - * thread different than it was created from. + * thread different than it was created from.

* * @see #close() * @see #checkNotClosed() @@ -57,20 +55,20 @@ public class AbstractAutoCloseableJdbcObject extends AbstractWrapper implements /** *

* Closing a parent object should close all the derived resources: this - * {@code Set} is used to maintain live references to objects created - * by the actual subclass.

+ * {@code Set} is used to maintain references to live (not closed) + * objects created by the actual subclass.

* *

* This {@code Set} only contains live children: child objects de-register * themselves when they are closed.

* *

- * Note: We use wrap {@code HashSet} to {@code synchronizedSet} so that - * operations will be thread-safe.

+ * Note: this {@code HashSet} is NOT thread-safe: all operations the access + * it are MANDATED to use external locking.

* * @see #close() */ - private final Set children = buildChildrenSet(() -> onLastChildRemoved()); + private final Set children = new HashSet<>(); /** * We maintain a reference to the parent object, so that we can @@ -89,35 +87,6 @@ protected AbstractAutoCloseableJdbcObject(AbstractAutoCloseableJdbcObject parent this.parent = parent; } - /** - *

- * Constructs a new {@code Set}, which notifies {@code this} - * {@code AbstractAutoCloseableJdbcObject} when the last element - * is removed.

- * - * @param onLastElementRemoved the callback to call, when all children are closed - * - * @return a {@code Set}, which notifies {@code this}, when the last element is removed from it - */ - private Set buildChildrenSet(Runnable onLastElementRemoved) { - - Set actualStoreSet = new HashSet<>(); - - RemoveLastItemCallbackSet removeLastItemCallbackSet = - new RemoveLastItemCallbackSet(actualStoreSet, onLastElementRemoved); - - /* We wrap the Set, which provides the callback to avoid - * race conditions: the wrapper synchronizedSet HOLDS the - * monitor while the callback is being executed, hence a - * new registration cannot occur during the remove calls. - */ - return Collections.synchronizedSet(removeLastItemCallbackSet); - } - - protected void onLastChildRemoved() { - // template method for sub-classes to override - } - /** *

* Throws {@code SQLException} if {@code this} is closed. @@ -165,18 +134,23 @@ public final boolean isClosed() { * @throws SQLException in case closing {@code this} object or any of the the child object fails */ public final void close() throws SQLException { - final boolean closedFlagSet = closed.compareAndSet(false, true); - if (closedFlagSet) { + final boolean closedFlagWasSetDuringThisMethodCall = closed.compareAndSet(false, true); + if (closedFlagWasSetDuringThisMethodCall) { Throwable parentCloseThrowable = null; if (parent != null) { + try { - parent.children.remove(this); - } catch (RuntimeDyna4JdbcException ex) { - // Unwrap actual exception thrown from - // AbstractStatement#onLastChildRemoved() - parentCloseThrowable = ex.getCause(); + parent.unregisterChild(this); + } catch (Throwable thowable) { + /* Normally, any Throwable caught here should be a + * SQLException, however, we catch all Throwables + * so as to handle offending implementations properly: + * if a RuntimeException or Error is thrown, we still + * catch and pass it to method #closeChildObjects(Throwable) + */ + parentCloseThrowable = thowable; } } @@ -194,44 +168,48 @@ public final void close() throws SQLException { * @throws SQLException if one or more child objects throw {@code Throwable} on close method call */ private void closeChildObjects(Throwable parentCloseException) throws SQLException { - LinkedList caughtThrowables = new LinkedList<>(); - - for (AutoCloseable closeableObject : children) { - - try { - closeableObject.close(); - } catch (Throwable closeThrowable) { - /* Normally, any Throwable caught here should be a - * SQLException, however, we catch all Throwables - * so as to handle offending implementations properly: - * if a RuntimeException or Error is thrown, we still - * catch it and wrap into a SQLException - */ - caughtThrowables.add(closeThrowable); + + synchronized (this.children) { + LinkedList caughtThrowables = new LinkedList<>(); + + for (AutoCloseable closeableObject : this.children) { + + try { + closeableObject.close(); + } catch (Throwable closeThrowable) { + /* Normally, any Throwable caught here should be a + * SQLException, however, we catch all Throwables + * so as to handle offending implementations properly: + * if a RuntimeException or Error is thrown, we still + * catch it and wrap into a SQLException + */ + caughtThrowables.add(closeThrowable); + } } - } - children.clear(); // closing also includes discarding references + children.clear(); // closing also includes discarding references - if (parentCloseException != null) { - caughtThrowables.add(parentCloseException); - } + if (parentCloseException != null) { + caughtThrowables.add(parentCloseException); + } - switch (caughtThrowables.size()) { - case 0: - return; // no Throwable caught: normal completion + switch (caughtThrowables.size()) { + case 0: + return; // no Throwable caught: normal completion - case 1: - // closure of a single child has failed: propagate it as the root cause - throw JDBCError.CLOSE_FAILED.raiseSQLException(caughtThrowables.getFirst(), - this, "Closing of dependent object caused exception"); + case 1: + // closure of a single child has failed: propagate it as the root cause + throw JDBCError.CLOSE_FAILED.raiseSQLException(caughtThrowables.getFirst(), + this, "Closing of dependent object caused exception"); - default: - // closure of multiple children has failed: propagate them as suppressed - throw JDBCError.CLOSE_FAILED.raiseSQLExceptionWithSupressed(caughtThrowables, - this, "Closing of dependent objects caused exceptions; see supressed"); + default: + // closure of multiple children has failed: propagate them as suppressed + throw JDBCError.CLOSE_FAILED.raiseSQLExceptionWithSupressed(caughtThrowables, + this, "Closing of dependent objects caused exceptions; see supressed"); - } + } + + } // end of synchronized (this.children) block } @@ -244,17 +222,32 @@ private void closeChildObjects(Throwable parentCloseException) throws SQLExcepti * @throws RuntimeException if {@code this} is closed already */ protected final void registerAsChild(AutoCloseable closableObject) { - if (isClosed()) { - JDBCError.DRIVER_BUG_UNEXPECTED_STATE.raiseUncheckedException( - "Attempt to register a child to a closed object"); - } if (closableObject == null) { JDBCError.DRIVER_BUG_UNEXPECTED_STATE.raiseUncheckedException( "closableObject to register cannot be null"); } - children.add(closableObject); + /* We do not even try to acquire the monitor of this.children + * in case this object is closed. This prevents potentially + * unnecessary wait on the monitor if the registration cannot + * be performed anyways. However, the check HAS TO BE EXECUTED + * once again, when the monitor is actually held. + */ + if (isClosed()) { + JDBCError.DRIVER_BUG_UNEXPECTED_STATE.raiseUncheckedException( + "Attempt to register a child to a closed object"); + } + + synchronized (this.children) { + + if (isClosed()) { + JDBCError.DRIVER_BUG_UNEXPECTED_STATE.raiseUncheckedException( + "Attempt to register a child to a closed object"); + } + + children.add(closableObject); + } // end of synchronized(this.children) block } /** @@ -270,4 +263,42 @@ public final void registerAsChildren(Collection closabl registerAsChild(aClosableSQLObject); } } + + /** + * Unregisters a child object from {@code this} object. If there is no more + * child element is registered after the removal of the supplied object, then + * the template method {@link #onLastChildRemoved()} is also invoked. + * + * @param childObject the child object to remove from registration + * + * @throws SQLException in case {@link #onLastChildRemoved()} was invoked and it threw an exception + */ + private void unregisterChild(AutoCloseable childObject) throws SQLException { + + synchronized (this.children) { + boolean setContainedTheElement = this.children.remove(childObject); + if (!setContainedTheElement) { + /* Added out of paranoia: should never happen. + * Something is fundamentally wrong, if this occurs: we are + * attempting to un-register a child, which was never ours? + * Fail fast by throwing an exception! + */ + throw JDBCError.DRIVER_BUG_UNEXPECTED_STATE.raiseUncheckedException( + "Child to unregister was not found as a child."); + } + + if (this.children.isEmpty()) { + /* NOTE: we invoke the callback while holding the monitor of + * this.children. Since this is mutually exclusive with the + * addition of a new child, the callback is guaranteed to + * see a state, where this.children.isEmpty() is true + */ + onLastChildRemoved(); + } + } // end of synchronized(this.children) block + } + + protected void onLastChildRemoved() throws SQLException { + // template method for sub-classes to override + } } diff --git a/src/main/java/com/github/dyna4jdbc/internal/common/util/collection/RemoveLastItemCallbackSet.java b/src/main/java/com/github/dyna4jdbc/internal/common/util/collection/RemoveLastItemCallbackSet.java deleted file mode 100644 index 4f0d53a..0000000 --- a/src/main/java/com/github/dyna4jdbc/internal/common/util/collection/RemoveLastItemCallbackSet.java +++ /dev/null @@ -1,39 +0,0 @@ -package com.github.dyna4jdbc.internal.common.util.collection; - -import java.util.Collection; -import java.util.Set; - -public final class RemoveLastItemCallbackSet extends SetProxy { - - private final Runnable onLastItemRemovedCallback; - - public RemoveLastItemCallbackSet(Set delegate, Runnable runnable) { - super(delegate); - this.onLastItemRemovedCallback = runnable; - } - - @Override - public boolean remove(Object o) { - boolean returnValue = super.remove(o); - if (isEmpty()) { - onLastItemRemovedCallback.run(); - } - return returnValue; - } - - public boolean retainAll(Collection c) { - boolean returnValue = super.retainAll(c); - if (isEmpty()) { - onLastItemRemovedCallback.run(); - } - return returnValue; - } - - public boolean removeAll(Collection c) { - boolean returnValue = super.removeAll(c); - if (isEmpty()) { - onLastItemRemovedCallback.run(); - } - return returnValue; - } -} diff --git a/src/main/java/com/github/dyna4jdbc/internal/common/util/collection/SetProxy.java b/src/main/java/com/github/dyna4jdbc/internal/common/util/collection/SetProxy.java deleted file mode 100644 index 078056e..0000000 --- a/src/main/java/com/github/dyna4jdbc/internal/common/util/collection/SetProxy.java +++ /dev/null @@ -1,93 +0,0 @@ -package com.github.dyna4jdbc.internal.common.util.collection; - -import java.util.Collection; -import java.util.Iterator; -import java.util.Set; -import java.util.Spliterator; -import java.util.function.Consumer; -import java.util.function.Predicate; -import java.util.stream.Stream; - -public class SetProxy implements Set { - - private final Set delegate; - - public SetProxy(Set delegate) { - this.delegate = delegate; - } - - //CHECKSTYLE.OFF: DesignForExtension - public void forEach(Consumer action) { - delegate.forEach(action); - } - - public int size() { - return delegate.size(); - } - - public boolean isEmpty() { - return delegate.isEmpty(); - } - - public boolean contains(Object o) { - return delegate.contains(o); - } - - public Iterator iterator() { - return delegate.iterator(); - } - - public Object[] toArray() { - return delegate.toArray(); - } - - public T[] toArray(T[] a) { - return delegate.toArray(a); - } - - public boolean add(E e) { - return delegate.add(e); - } - - public boolean remove(Object o) { - return delegate.remove(o); - } - - public boolean containsAll(Collection c) { - return delegate.containsAll(c); - } - - public boolean addAll(Collection c) { - return delegate.addAll(c); - } - - public boolean retainAll(Collection c) { - return delegate.retainAll(c); - } - - public boolean removeAll(Collection c) { - return delegate.removeAll(c); - } - - public void clear() { - delegate.clear(); - } - - public Spliterator spliterator() { - return delegate.spliterator(); - } - - public boolean removeIf(Predicate filter) { - return delegate.removeIf(filter); - } - - public Stream stream() { - return delegate.stream(); - } - - public Stream parallelStream() { - return delegate.parallelStream(); - } - - //CHECKSTYLE.ON: DesignForExtension -}