Index: java/client/org/apache/derby/client/net/NetConnection.java IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- java/client/org/apache/derby/client/net/NetConnection.java (revision 1794094) +++ java/client/org/apache/derby/client/net/NetConnection.java (revision ) @@ -391,7 +391,7 @@ // This prevents attempts to use this closed connection // to retrieve error message text if an error SQLCA // is returned in one of the connect flows. - open_ = false; + setOpen(false); handleLoginTimeout( e ); @@ -440,7 +440,7 @@ // This prevents attempts to use this closed connection // to retrieve error message text if an error SQLCA // is returned in one of the connect flows. - open_ = false; + setOpen(false); handleLoginTimeout( e ); @@ -533,7 +533,7 @@ securityMechanism); } } catch (SqlException sqle) { // this may not be needed because on method up the stack - open_ = false; // all reset exceptions are caught and wrapped in disconnect exceptions + setOpen(false); // all reset exceptions are caught and wrapped in disconnect exceptions try { if (agent_ != null) { agent_.close(); @@ -1730,9 +1730,9 @@ * @return open */ public boolean isOpen() { - return open_; + return super.isOpen(); } - + /** * Invokes write commit on NetXAConnection */ Index: java/client/org/apache/derby/client/net/NetAgent.java IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- java/client/org/apache/derby/client/net/NetAgent.java (revision 1794094) +++ java/client/org/apache/derby/client/net/NetAgent.java (revision ) @@ -121,6 +121,8 @@ LogWriter logWriter) throws SqlException { super(netConnection, logWriter); this.netConnection_ = netConnection; + // capture fully initialized state of NetAgent + getCleanupAction().afterInit(this); } NetAgent(NetConnection netConnection, @@ -239,6 +241,8 @@ statementRequest_ = (StatementRequestInterface) netStatementRequest_; connectionRequest_ = (ConnectionRequestInterface) netConnectionRequest_; } + // capture fully initialized state of NetAgent + getCleanupAction().afterInit(this); } protected void resetAgent_(LogWriter netLogWriter, @@ -289,7 +293,22 @@ // Close socket and its streams. public void close_() throws SqlException { - // can we just close the socket here, do we need to close streams individually + SqlException accumulatedExceptions = + close_(rawSocketInputStream_, rawSocketOutputStream_, socket_, logWriter_); + rawSocketInputStream_ = null; + rawSocketOutputStream_ = null; + socket_ = null; + if (accumulatedExceptions != null) { + throw accumulatedExceptions; + } + } + + private static SqlException close_(InputStream rawSocketInputStream_, + OutputStream rawSocketOutputStream_, + Socket socket_, + LogWriter logWriter_) + { + // can we just close the socket here, do we need to close streams individually SqlException accumulatedExceptions = null; if (rawSocketInputStream_ != null) { try { @@ -302,8 +321,6 @@ accumulatedExceptions = new SqlException(logWriter_, new ClientMessageId(SQLState.COMMUNICATION_ERROR), e, e.getMessage()); - } finally { - rawSocketInputStream_ = null; } } @@ -319,8 +336,6 @@ new ClientMessageId(SQLState.COMMUNICATION_ERROR), e, e.getMessage()); accumulatedExceptions = Utils.accumulateSQLException(latestException, accumulatedExceptions); - } finally { - rawSocketOutputStream_ = null; } } @@ -336,14 +351,10 @@ new ClientMessageId(SQLState.COMMUNICATION_ERROR), e, e.getMessage()); accumulatedExceptions = Utils.accumulateSQLException(latestException, accumulatedExceptions); - } finally { - socket_ = null; } } - if (accumulatedExceptions != null) { - throw accumulatedExceptions; - } + return accumulatedExceptions; } /** @@ -422,11 +433,11 @@ } void setInputStream(InputStream inputStream) { - rawSocketInputStream_ = inputStream; + getCleanupAction().setRawSocketInputStream(rawSocketInputStream_ = inputStream); } void setOutputStream(OutputStream outputStream) { - rawSocketOutputStream_ = outputStream; + getCleanupAction().setRawSocketOutputStream(rawSocketOutputStream_ = outputStream); } void throwCommunicationsFailure(Throwable cause) @@ -547,6 +558,55 @@ void switchToEbcdicMgr() { currentCcsidManager_ = ebcdicCcsidManager_; } + + // cleanup action construction + + + @Override + protected NetCleanupAction getCleanupAction() { + return (NetCleanupAction) super.getCleanupAction(); + } + + @Override + protected CleanupAction createCleanupAction() { + return new NetCleanupAction(this); + } + + protected static class NetCleanupAction extends CleanupAction { + private InputStream rawSocketInputStream; + private OutputStream rawSocketOutputStream; + private Socket socket; // socket is either null or stable non-null + + protected NetCleanupAction(NetAgent agent) { + super(agent); + } + + // invoked from the end of every NetAgent constructor when the state is already initialized... + private void afterInit(NetAgent agent) { + rawSocketInputStream = agent.rawSocketInputStream_; + rawSocketOutputStream = agent.rawSocketOutputStream_; + socket = agent.socket_; + } + + // rawSocketInputStream may be changed during the lifetime of the NetAgent instance + private void setRawSocketInputStream(InputStream rawSocketInputStream) { + this.rawSocketInputStream = rawSocketInputStream; + } + + // rawSocketOutputStream may be changed during the lifetime of the NetAgent instance + private void setRawSocketOutputStream(OutputStream rawSocketOutputStream) { + this.rawSocketOutputStream = rawSocketOutputStream; + } + + @Override + public void run() { + // execute NetAgent's part of cleanup acrion + SqlException ignored = + close_(rawSocketInputStream, rawSocketOutputStream, socket, getLogWriter()); + // invoke Agent's part of cleanup action after that + super.run(); + } + } } Index: java/client/org/apache/derby/client/am/ClientConnection.java IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- java/client/org/apache/derby/client/am/ClientConnection.java (revision 1794094) +++ java/client/org/apache/derby/client/am/ClientConnection.java (revision ) @@ -21,6 +21,7 @@ package org.apache.derby.client.am; +import java.lang.ref.Cleaner; import java.sql.Blob; import java.sql.CallableStatement; import java.sql.Clob; @@ -38,6 +39,7 @@ import java.util.Hashtable; import java.util.Iterator; import java.util.Map; +import java.util.Objects; import java.util.Properties; import java.util.Set; import java.util.WeakHashMap; @@ -51,6 +53,10 @@ public abstract class ClientConnection implements Connection, ConnectionCallbackInterface { + // single Cleaner for all ClientConnection(s) + // should in reality be shared among different classes needing its support... + private static final Cleaner cleaner = Cleaner.create(); + //---------------------navigational members----------------------------------- @@ -58,11 +64,11 @@ public ClientDatabaseMetaData databaseMetaData_; // DERBY-210 - WeakHashMap is used to store references to objects to avoid - // memory leaks. When there are no other references to the keys in a - // WeakHashMap, they will get removed from the map and can thus get - // garbage-collected. They do not have to wait till the Connection object + // memory leaks. When there are no other references to the keys in a + // WeakHashMap, they will get removed from the map and can thus get + // garbage-collected. They do not have to wait till the Connection object // is collected. - + // In Connection.markStatementsClosed() method, this list is traversed to get a // list of open statements, which are marked closed and removed from the list. final WeakHashMap openStatements_ = @@ -117,6 +123,33 @@ // ------------------------dynamic properties--------------------------------- + + /** + * Use this method in combination with {@link #setOpen(boolean)} instead of + * directly accessing the {@link #open_} field... + * + * @return the value of open flag + */ + @SuppressWarnings("deprecation") + protected boolean isOpen() { + return open_; + } + + /** + * Use this method in combination with {@link #isOpen()} instead of + * directly accessing the {@link #open_} field... + * + * @param open the value to set the open flag to + */ + @SuppressWarnings("deprecation") + protected void setOpen(boolean open) { + cleanupAction.setOpen(open_ = open); + } + + /** + * @deprecated use {@link #isOpen()} / {@link #setOpen(boolean)} instead. + */ + @Deprecated protected boolean open_ = true; private boolean aborting_ = false; private boolean availableForReuse_ = false; @@ -212,6 +245,7 @@ this.user_ = user; initConnection(logWriter, dataSource); + cleanable = cleaner.register(this, cleanupAction = createCleanupAction()); } protected ClientConnection( @@ -225,7 +259,8 @@ this.user_ = user; isXAConnection_ = isXAConn; initConnection(logWriter, dataSource); - } + cleanable = cleaner.register(this, cleanupAction = createCleanupAction()); + } // For jdbc 2 connections private void initConnection( @@ -312,6 +347,8 @@ serverNameIP_, portNumber_, clientSSLMode_); + + cleanable = cleaner.register(this, cleanupAction = createCleanupAction()); } // This is a callback method, called by subsystem - NetConnection @@ -363,37 +400,86 @@ serverNameIP_, portNumber_, clientSSLMode_); + + cleanable = cleaner.register(this, cleanupAction = createCleanupAction()); } + + // cleanup action construction // Users are advised to call the method close() on Statement and Connection objects when they are done with them. // However, some users will forget, and some code may get killed before it can close these objects. // Therefore, if JDBC drivers have state associated with JDBC objects that need to get - // explicitly cleared up, they should provide finalize methods to take care of them. - // The garbage collector will call these finalize methods when the objects are found to be garbage, + // explicitly cleared up, they should provide cleanup actions to take care of them. + // The garbage collector will call these cleanup actions when the objects are found to be garbage, // and this will give the driver a chance to close (or otherwise clean up) the objects. // Note, however, that there is no guarantee that the garbage collector will ever run. - // If that is the case, the finalizers will not be called. - protected void finalize() throws Throwable { - if (agent_.loggingEnabled()) { - agent_.logWriter_.traceEntry(this, "finalize"); - } + // If that is the case, the cleanup actions will not be called. + + private final Cleaner.Cleanable cleanable; + private final CleanupAction cleanupAction; + + /** + * Creates a cleanup action object capturing connection state needed to + * perform the cleanup. Subclasses may override this method and return + * a subclass of {@link CleanupAction} with additional state and/or behavior. + * @return a CleanupAction instance + */ + protected CleanupAction createCleanupAction() { + return new CleanupAction(this); + } - // finalize() differs from close() in that it will not throw an - // exception if a transaction is in progress. - // finalize() also differs from close() in that it will not drive - // an auto-commit before disconnecting. - // - // If a transaction is in progress, a close() request will throw an SqlException. - // However, if a connection with an incomplete transaction is finalized, - // or is abruptly terminated by application exit, - // the normal rollback semantics imposed by the DERBY server are adopted. - // So we just pull the plug and let the server handle this default semantic. + protected static class CleanupAction implements Runnable { + private final WeakHashMap openStatements; + private final WeakHashMap commitAndRollbackListeners; + private final Agent.CleanupAction agentCleanupAction; + private boolean open; + + protected CleanupAction(ClientConnection connection) { + openStatements = Objects.requireNonNull(connection.openStatements_); + commitAndRollbackListeners = Objects.requireNonNull(connection.CommitAndRollbackListeners_); + agentCleanupAction = Objects.requireNonNull(connection.agent_.getCleanupAction()); + open = connection.isOpen(); + } + + // open flag may change during the lifetime of the ClientConnection instance + // (when user explicitly closes the connection) + private void setOpen(boolean open) { + this.open = open; + } + + /** + * This mimics old finalize(). + */ + @Override + public void run() { + if (agentCleanupAction.getLogWriter() != null) { + agentCleanupAction.getLogWriter().traceEntry(this, "run"); + } + + // cleanup action differs from close() in that it will not throw an + // exception if a transaction is in progress. + // cleanup action also differs from close() in that it will not drive + // an auto-commit before disconnecting. + // + // If a transaction is in progress, a close() request will throw an SqlException. + // However, if a connection with an incomplete transaction is cleaned up, + // or is abruptly terminated by application exit, + // the normal rollback semantics imposed by the DERBY server are adopted. + // So we just pull the plug and let the server handle this default semantic. - if (!open_) { - return; + // perform cleanup only if still open + if (open) { + // invoke agent cleanup action + agentCleanupAction.run(); + + // following mimics actions that were run as agent's disconnectEvent() + // callback (i.e. completeChainBreakingDisconnect()), but are + // now explicitly invoked after agent cleanup action finishes... + open = false; + completeLocalRollback(commitAndRollbackListeners); + markStatementsClosed(openStatements); + } } - agent_.disconnectEvent(); - super.finalize(); } // ---------------------------jdbc 1------------------------------------------ @@ -804,7 +890,7 @@ // This is a no-op if the connection is already closed. private void closeX() throws SQLException { - if (!open_ && !isAborting()) { + if (!isOpen() && !isAborting()) { return; } closeResourcesX(); @@ -813,7 +899,7 @@ // Close physical socket or attachment even if connection is marked close. // Used by ClientPooledConnection.close(). synchronized public void closeResources() throws SQLException { - if (open_ || (!open_ && availableForReuse_)) { + if (isOpen() || (!isOpen() && availableForReuse_)) { availableForReuse_ = false; closeResourcesX(); } @@ -869,6 +955,10 @@ throw Utils.accumulateSQLException(e.getSQLException(), accumulatedExceptions); } + + // trigger cleanup action so that it gets de-registered early + // (it will be a no-op since the connection is already marked as closed) + cleanable.clean(); } protected abstract boolean isGlobalPending_(); @@ -877,7 +967,7 @@ // Physical resources are not closed. synchronized void closeForReuse(boolean statementPooling) throws SqlException { - if (!open_) { + if (!isOpen()) { return; } resetConnectionAtFirstSql_ = false; // unset indicator of deferred reset @@ -887,7 +977,7 @@ } catch (SqlException e) { accumulatedExceptions = e; } - if (open_) { + if (isOpen()) { markClosedForReuse(statementPooling); } if (accumulatedExceptions != null) { @@ -917,7 +1007,7 @@ private void markClosed(boolean statementPooling) { - open_ = false; + setOpen(false); inUnitOfWork_ = false; if (!statementPooling) { markStatementsClosed(); @@ -933,7 +1023,11 @@ } private void markStatementsClosed() { - Set keySet = openStatements_.keySet(); + markStatementsClosed(openStatements_); + } + + private static void markStatementsClosed(WeakHashMap openStatements) { + Set keySet = openStatements.keySet(); for (Iterator i = keySet.iterator(); i.hasNext();) { ClientStatement stmt = i.next(); stmt.markClosed(); @@ -961,20 +1055,20 @@ * @return true if physical connection still open */ public boolean isPhysicalConnClosed() { - return !open_ && !availableForReuse_; + return !isOpen() && !availableForReuse_; } public boolean isClosed() { if (agent_.loggingEnabled()) { - agent_.logWriter_.traceExit(this, "isClosed", !open_); + agent_.logWriter_.traceExit(this, "isClosed", !isOpen()); } - return !open_; + return !isOpen(); } public boolean isClosedX() { - return !open_; + return !isOpen(); } private static String DERBY_TRANSACTION_REPEATABLE_READ = "RS"; @@ -2155,7 +2249,7 @@ public final void completeConnect() throws SqlException { - open_ = true; + setOpen(true); databaseMetaData_ = newDatabaseMetaData_(); agent_.sectionManager_ = @@ -2206,14 +2300,18 @@ // This is a client-side only operation. // This method will only throw an exception on bug check. public void completeLocalRollback() { - Set keySet = CommitAndRollbackListeners_.keySet(); - for (Iterator i = keySet.iterator(); i.hasNext();) { - i.next().completeLocalRollback(i); - } + completeLocalRollback(CommitAndRollbackListeners_); inUnitOfWork_ = false; transactionID_++; } - + + private static void completeLocalRollback(WeakHashMap commitAndRollbackListeners) { + Set keySet = commitAndRollbackListeners.keySet(); + for (Iterator i = keySet.iterator(); i.hasNext(); ) { + i.next().completeLocalRollback(i); + } + } + /** * * Rollback the specific UnitOfWorkListener. @@ -2277,7 +2375,7 @@ // This is a client-side only operation. // This method will only throw an exception if the agent cannot be closed. public void completeChainBreakingDisconnect() { - open_ = false; + setOpen(false); completeLocalRollback(); markStatementsClosed(); } @@ -2363,10 +2461,10 @@ } synchronized public void lightReset() throws SqlException { - if (!open_ && !availableForReuse_) { + if (!isOpen() && !availableForReuse_) { return; } - open_ = true; + setOpen(true); availableForReuse_ = false; } @@ -2391,7 +2489,7 @@ boolean closeStatementsOnClose, NetXAResource xares) throws SqlException { - open_ = true; + setOpen(true); completeLocalRollback(); // this will close the cursors if the physical connection hadn't been closed for reuse properly @@ -2454,7 +2552,7 @@ //-------------------------------helper methods------------------------------- protected void checkForClosedConnection() throws SqlException { - if (!open_) { + if (!isOpen()) { agent_.checkForDeferredExceptions(); throw new SqlException(agent_.logWriter_, new ClientMessageId (SQLState.NO_CURRENT_CONNECTION)); @@ -2688,7 +2786,7 @@ public void abort( Executor executor ) throws SQLException { // NOP if called on a closed connection. - if ( !open_ ) { return; } + if ( !isOpen() ) { return; } // Null executor not allowed. if ( executor == null ) { Index: java/client/org/apache/derby/client/am/Agent.java IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- java/client/org/apache/derby/client/am/Agent.java (revision 1794094) +++ java/client/org/apache/derby/client/am/Agent.java (revision ) @@ -24,6 +24,7 @@ import java.io.PrintWriter; import java.sql.BatchUpdateException; import java.sql.Types; +import java.util.Objects; import org.apache.derby.jdbc.ClientDriver; import org.apache.derby.shared.common.reference.JDBC40Translation; import org.apache.derby.shared.common.reference.SQLState; @@ -35,7 +36,7 @@ private int batchedExceptionLabelIndex_; private boolean[] batchedExceptionGenerated_; - ClientConnection connection_; // made friendly for lobs only, refactor !! + final ClientConnection connection_; // made friendly for lobs only, refactor !! SectionManager sectionManager_ = null; @@ -112,6 +113,10 @@ connection_ = connection; logWriter_ = logWriter; crossConverters_ = new CrossConverters(this); + + // no Cleaner registration here, since ClientConnection's cleanup + // action delegates to this cleanup action... + cleanupAction = createCleanupAction(); } private void resetAgent(LogWriter logWriter) { @@ -120,7 +125,7 @@ enableBatchedExceptionTracking_ = false; batchedExceptionLabelIndex_ = 0; batchedExceptionGenerated_ = null; - logWriter_ = logWriter; + cleanupAction.setLogWriter(logWriter_ = logWriter); deferredException_ = null; } @@ -131,6 +136,8 @@ String server, int port) throws SqlException { + assert this.connection_ == connection; + resetAgent(logWriter); resetAgent_(logWriter, loginTimeout, server, port); } @@ -148,7 +155,7 @@ if (logWriter_ != null) { logWriter_.close(); } - logWriter_ = logWriter; + cleanupAction.setLogWriter(logWriter_ = logWriter); } } @@ -329,6 +336,43 @@ null, updateCounts, accumulatedExceptions); } } + + // cleanup action construction + + private final CleanupAction cleanupAction; + + protected CleanupAction getCleanupAction() { + return cleanupAction; + } + + // invoked from the end of Agent constructor + protected CleanupAction createCleanupAction() { + return new CleanupAction(this); + } + + protected static class CleanupAction implements Runnable { + private LogWriter logWriter; + + protected CleanupAction(Agent agent) { + logWriter = Objects.requireNonNull(agent.logWriter_); + } + + protected LogWriter getLogWriter() { + return logWriter; + } + + // logWriter may be changed during the lifetime of the Agent instance + private void setLogWriter(LogWriter logWriter) { + this.logWriter = logWriter; + } + + @Override + public void run() { + if (logWriter != null) { + logWriter.close(); + } + } + } }