Commit bd621842 authored by gmantele's avatar gmantele
Browse files

[TAP] New fix for the transaction management.

The transaction and Statement were closed too early before.
  - Fetching the row was not possible once the first bunch of fetched
rows was over.
  - The problem of "statement is aborted" preventing the re-use of
a same DB connection was apparently still there, but occurred less often.

  Now, any transaction potentially started in a DB connection is always
closed after one of the public functions of JDBCConnection is called ;
except executeQuery(ADQLQuery) whose the call MUST be wrapped inside a
try...catch block in which DBConnection.cancel(true) MUST be called
in case of error (in order to effectively end any started transaction).
parent b270eed3
Loading
Loading
Loading
Loading
+12 −9
Original line number Diff line number Diff line
@@ -16,7 +16,7 @@ package tap;
 * You should have received a copy of the GNU Lesser General Public License
 * along with TAPLibrary.  If not, see <http://www.gnu.org/licenses/>.
 * 
 * Copyright 2012-2015 - UDS/Centre de Données astronomiques de Strasbourg (CDS),
 * Copyright 2012-2016 - UDS/Centre de Données astronomiques de Strasbourg (CDS),
 *                       Astronomisches Rechen Institut (ARI)
 */

@@ -25,6 +25,10 @@ import java.io.OutputStream;

import javax.servlet.http.HttpServletResponse;

import adql.parser.ADQLParser;
import adql.parser.ADQLQueryFactory;
import adql.parser.ParseException;
import adql.query.ADQLQuery;
import tap.data.DataReadException;
import tap.data.TableIterator;
import tap.db.DBConnection;
@@ -41,10 +45,6 @@ import uws.UWSToolBox;
import uws.job.JobThread;
import uws.job.Result;
import uws.service.log.UWSLog.LogLevel;
import adql.parser.ADQLParser;
import adql.parser.ADQLQueryFactory;
import adql.parser.ParseException;
import adql.query.ADQLQuery;

/**
 * <p>Let process completely an ADQL query.</p>
@@ -104,7 +104,7 @@ import adql.query.ADQLQuery;
 * </p>
 * 
 * @author Gr&eacute;gory Mantelet (CDS;ARI)
 * @version 2.1 (11/2015)
 * @version 2.1 (04/2016)
 */
public class ADQLExecutor {

@@ -419,6 +419,9 @@ public class ADQLExecutor {

			// Free the connection (so that giving it back to a pool, if any, otherwise, just free resources):
			if (dbConn != null){
				// be sure no query is still processing and that any open transaction is rollbacked and ended:
				dbConn.cancel(true);
				// free the connection:
				service.getFactory().freeConnection(dbConn);
				dbConn = null;
			}
+5 −5
Original line number Diff line number Diff line
@@ -49,6 +49,9 @@ import javax.naming.InitialContext;
import javax.naming.NamingException;
import javax.sql.DataSource;

import adql.translator.JDBCTranslator;
import adql.translator.PgSphereTranslator;
import adql.translator.PostgreSQLTranslator;
import tap.AbstractTAPFactory;
import tap.ServiceConnection;
import tap.TAPException;
@@ -60,9 +63,6 @@ import uws.UWSException;
import uws.service.UWSService;
import uws.service.backup.UWSBackupManager;
import uws.service.log.UWSLog.LogLevel;
import adql.translator.JDBCTranslator;
import adql.translator.PgSphereTranslator;
import adql.translator.PostgreSQLTranslator;

/**
 * <p>Concrete implementation of a {@link TAPFactory} which is parameterized by a TAP configuration file.</p>
@@ -74,7 +74,7 @@ import adql.translator.PostgreSQLTranslator;
 * </p>
 * 
 * @author Gr&eacute;gory Mantelet (ARI)
 * @version 2.1 (02/2016)
 * @version 2.1 (04/2016)
 * @since 2.0
 */
public class ConfigurableTAPFactory extends AbstractTAPFactory {
@@ -282,7 +282,7 @@ public class ConfigurableTAPFactory extends AbstractTAPFactory {
	public void freeConnection(DBConnection conn){
		try{
			// Cancel any possible query that could be running:
			conn.cancel(false);
			conn.cancel(true);
			// Close the connection (if a connection pool is used, the connection is not really closed but is freed and kept in the pool for further usage):
			((JDBCConnection)conn).getInnerConnection().close();
		}catch(SQLException se){
+18 −17
Original line number Diff line number Diff line
@@ -16,7 +16,7 @@ package tap.data;
 * You should have received a copy of the GNU Lesser General Public License
 * along with TAPLibrary.  If not, see <http://www.gnu.org/licenses/>.
 * 
 * Copyright 2014-2015 - Astronomisches Rechen Institut (ARI)
 * Copyright 2014-2016 - Astronomisches Rechen Institut (ARI)
 */

import java.sql.ResultSet;
@@ -26,14 +26,14 @@ import java.sql.Statement;
import java.sql.Timestamp;
import java.util.NoSuchElementException;

import tap.metadata.TAPColumn;
import uws.ISO8601Format;
import adql.db.DBColumn;
import adql.db.DBType;
import adql.db.DBType.DBDatatype;
import adql.db.STCS.Region;
import adql.parser.ParseException;
import adql.translator.JDBCTranslator;
import tap.metadata.TAPColumn;
import uws.ISO8601Format;

/**
 * <p>{@link TableIterator} which lets iterate over a SQL {@link ResultSet}.</p>
@@ -43,7 +43,7 @@ import adql.translator.JDBCTranslator;
 * </i></p>
 * 
 * @author Gr&eacute;gory Mantelet (ARI)
 * @version 2.1 (11/2015)
 * @version 2.1 (04/2016)
 * @since 2.0
 */
public class ResultSetTableIterator implements TableIterator {
@@ -688,7 +688,8 @@ public class ResultSetTableIterator implements TableIterator {
			return new DBType(DBDatatype.UNKNOWN);

		// Extract the type prefix and lower-case it:
		int startParamIndex = dbmsTypeName.indexOf('('), endParamIndex = dbmsTypeName.indexOf(')');
		int startParamIndex = dbmsTypeName.indexOf('('),
				endParamIndex = dbmsTypeName.indexOf(')');
		String dbmsTypePrefix = (startParamIndex <= 0) ? dbmsTypeName : dbmsTypeName.substring(0, endParamIndex);
		dbmsTypePrefix = dbmsTypePrefix.trim().toLowerCase();
		String[] typeParams = (startParamIndex <= 0) ? null : dbmsTypeName.substring(startParamIndex + 1, endParamIndex).split(",");
+118 −103
Original line number Diff line number Diff line
@@ -16,7 +16,7 @@ package tap.db;
 * You should have received a copy of the GNU Lesser General Public License
 * along with TAPLibrary.  If not, see <http://www.gnu.org/licenses/>.
 * 
 * Copyright 2012-2015 - UDS/Centre de Données astronomiques de Strasbourg (CDS),
 * Copyright 2012-2016 - UDS/Centre de Données astronomiques de Strasbourg (CDS),
 *                       Astronomisches Rechen Institut (ARI)
 */

@@ -38,6 +38,16 @@ import java.util.List;
import java.util.Map;
import java.util.Properties;

import adql.db.DBColumn;
import adql.db.DBType;
import adql.db.DBType.DBDatatype;
import adql.db.STCS;
import adql.db.STCS.Region;
import adql.query.ADQLQuery;
import adql.query.IdentifierField;
import adql.translator.ADQLTranslator;
import adql.translator.JDBCTranslator;
import adql.translator.TranslationException;
import tap.data.DataReadException;
import tap.data.ResultSetTableIterator;
import tap.data.TableIterator;
@@ -52,16 +62,6 @@ import tap.metadata.TAPTable;
import tap.metadata.TAPTable.TableType;
import uws.ISO8601Format;
import uws.service.log.UWSLog.LogLevel;
import adql.db.DBColumn;
import adql.db.DBType;
import adql.db.DBType.DBDatatype;
import adql.db.STCS;
import adql.db.STCS.Region;
import adql.query.ADQLQuery;
import adql.query.IdentifierField;
import adql.translator.ADQLTranslator;
import adql.translator.JDBCTranslator;
import adql.translator.TranslationException;

/**
 * <p>This {@link DBConnection} implementation is theoretically able to deal with any DBMS JDBC connection.</p>
@@ -170,7 +170,7 @@ import adql.translator.TranslationException;
 * </i></p>
 * 
 * @author Gr&eacute;gory Mantelet (CDS;ARI)
 * @version 2.1 (12/2015)
 * @version 2.1 (04/2016)
 * @since 2.0
 */
public class JDBCConnection implements DBConnection {
@@ -455,6 +455,20 @@ public class JDBCConnection implements DBConnection {
		return connection;
	}

	/**
	 * <p>Tell whether this {@link JDBCConnection} is already associated with a {@link Statement}.</p>
	 * 
	 * @return	<code>true</code> if a {@link Statement} instance is already associated with this {@link JDBCConnection}
	 *        	<code>false</code> otherwise.
	 * 
	 * @throws SQLException	In case the open/close status of the current {@link Statement} instance can not be checked.
	 * 
	 * @since 2.1
	 */
	protected boolean hasStatement() throws SQLException{
		return (stmt != null && !stmt.isClosed());
	}

	/**
	 * <p>Get the only statement associated with this {@link JDBCConnection}.</p>
	 * 
@@ -470,10 +484,10 @@ public class JDBCConnection implements DBConnection {
	 * @since 2.1
	 */
	protected Statement getStatement() throws SQLException{
		if (stmt == null || stmt.isClosed())
			return (stmt = connection.createStatement(ResultSet.TYPE_FORWARD_ONLY, ResultSet.CONCUR_READ_ONLY));
		else
		if (hasStatement())
			return stmt;
		else
			return (stmt = connection.createStatement(ResultSet.TYPE_FORWARD_ONLY, ResultSet.CONCUR_READ_ONLY));
	}

	/**
@@ -496,12 +510,14 @@ public class JDBCConnection implements DBConnection {
	 * <p>
	 * 	If a call of this function fails the flag {@link #supportsCancel} is set to false
	 * 	so that any subsequent call of this function for this instance of {@link JDBCConnection}
	 * 	does not try any other cancellation attempt.
	 * 	does not try any other cancellation attempt. <b>HOWEVER</b> the rollback will still continue
	 * 	to be performed if the parameter <code>rollback</code> is set to <code>true</code>.
	 * </p>
	 * 
	 * <p><i>Note 1:
	 * 	A failure of a rollback is not considered as a not supported cancellation feature by the JDBC driver or the DBMS.
	 * 	So if the cancellation succeeds but a rollback fails, a next call of this function will still try cancelling the given statement.
	 * 	In case of a rollback failure, only a WARNING is written in the log file ; no exception is thrown.
	 * </i></p>
	 * 
	 * <p><i>Note 2:
@@ -515,7 +531,8 @@ public class JDBCConnection implements DBConnection {
	 * 	Thus, it may block until another synchronized block on this same flag is finished.
	 * </i></p>
	 * 
	 * @param rollback	The statement to cancel. <i>Note: if closed or NULL, nothing will be done and no exception will be thrown.</i>
	 * @param rollback	<code>true</code> to cancel the statement AND rollback the current connection transaction,
	 *                	<code>false</code> to just cancel the statement.
	 * 
	 * @see DBConnection#cancel(boolean)
	 * @see #cancel(Statement, boolean)
@@ -544,15 +561,17 @@ public class JDBCConnection implements DBConnection {
	 * <p>
	 * 	If a call of this function fails the flag {@link #supportsCancel} is set to false
	 * 	so that any subsequent call of this function for this instance of {@link JDBCConnection}
	 * 	does not try any other cancellation attempt.
	 * 	does not try any other cancellation attempt. <b>HOWEVER</b> the rollback will still continue
	 * 	to be performed if the parameter <code>rollback</code> is set to <code>true</code>.
	 * </p>
	 * 
	 * <p><i>Note:
	 * 	A failure of a rollback is not considered as a not supported cancellation feature by the JDBC driver or the DBMS.
	 * 	So if the cancellation succeeds but a rollback fails, a next call of this function will still try canceling the given statement.
	 * 	In case of a rollback failure, only a WARNING is written in the log file ; no exception is thrown.
	 * </i></p>
	 * 
	 * @param stmt		The statement to cancel. <i>Note: if closed or NULL, nothing will be done and no exception will be thrown.</i>
	 * @param stmt		The statement to cancel. <i>Note: if closed or NULL, no exception will be thrown and only a rollback will be attempted if asked in parameter.</i>
	 * @param rollback	<code>true</code> to cancel the statement AND rollback the current connection transaction,
	 *                	<code>false</code> to just cancel the statement.
	 * 
@@ -562,31 +581,14 @@ public class JDBCConnection implements DBConnection {
	 * @since 2.1
	 */
	protected boolean cancel(final Statement stmt, final boolean rollback){
		// Not supported "cancel" operation => fail!
		if (!supportsCancel)
			return false;

		// No statement => "cancellation" successful!
		if (stmt == null)
			return true;

		// If the statement is not already closed, cancel its current query execution:
		try{
			if (!stmt.isClosed()){
				// Cancel the query execution:
			// If the statement is not already closed, cancel its current query execution:
			if (supportsCancel && stmt != null && !stmt.isClosed()){
				stmt.cancel();
				// Rollback all executed operations (only if in a transaction ; that's to say if AutoCommit = false):
				if (rollback && supportsTransaction){
					try{
						if (!connection.getAutoCommit())
							connection.rollback();
					}catch(SQLException se){
						if (logger != null)
							logger.logDB(LogLevel.ERROR, this, "CANCEL", "Query execution successfully stopped BUT the rollback fails!", se);
					}
				}
			}
				return true;
			}else
				return false;

		}catch(SQLFeatureNotSupportedException sfnse){
			// prevent further cancel attempts:
			supportsCancel = false;
@@ -600,6 +602,20 @@ public class JDBCConnection implements DBConnection {
				logger.logDB(LogLevel.ERROR, this, "CANCEL", "Abortion of the current query apparently fails! The query may still run on the database server.", se);
			return false;
		}
		// Whatever happens, rollback all executed operations (only if rollback=true and if in a transaction ; that's to say if AutoCommit = false):
		finally{
			if (rollback && supportsTransaction){
				try{
					if (!connection.getAutoCommit()){
						connection.rollback();
						connection.setAutoCommit(true);
					}
				}catch(SQLException se){
					if (logger != null)
						logger.logDB(LogLevel.ERROR, this, "CANCEL", "Query execution successfully stopped BUT the rollback fails!", se);
				}
			}
		}
	}

	/**
@@ -690,52 +706,38 @@ public class JDBCConnection implements DBConnection {
				logger.logDB(LogLevel.INFO, this, "RESULT", "Returning result (" + (supportsFetchSize ? "fetch size = " + fetchSize : "all in once") + ").", null);
			return createTableIterator(result, adqlQuery.getResultingColumns());

		}catch(SQLException se){
		}catch(Exception ex){
			// Close the ResultSet, if one was open:
			close(result);
			closeStatement();
			// Ensure the query is not running anymore and rollback the transaction:
			cancel(true);
			// If still not canceled, log this fact as an error:
			if (!isCancelled() && logger != null)
				logger.logDB(LogLevel.ERROR, this, "EXECUTE", "Unexpected error while EXECUTING SQL query!", null);
			throw new DBException("Unexpected error while executing a SQL query: " + se.getMessage(), se);
		}catch(TranslationException te){
			close(result);
			closeStatement();
			if (logger != null)
				logger.logDB(LogLevel.ERROR, this, "TRANSLATE", "Unexpected error while TRANSLATING ADQL into SQL!", null);
			throw new DBException("Unexpected error while translating ADQL into SQL: " + te.getMessage(), te);
		}catch(DataReadException dre){
			close(result);
			// Close the open statement, if one was open:
			closeStatement();
			if (logger != null)
				logger.logDB(LogLevel.ERROR, this, "RESULT", "Unexpected error while reading the query result!", null);
			throw new DBException("Impossible to read the query result, because: " + dre.getMessage(), dre);
		}finally{
			/* End the "transaction" eventually started when setting the fetch size (see 2. above).
			 * Note: this is particularly important when the query fails and the connection
			 *       is reused just after ; in this case, the failed query is not rollbacked/committed
			 *       and the following query fails before even starting because
			 *       the transaction is not ended. Some users got problem without this. */
			if (supportsFetchSize && fetchSize > 0){
				rollback();
				endTransaction();
			}
			// Propagate the exception with an appropriate error message:
			if (ex instanceof SQLException)
				throw new DBException("Unexpected error while executing a SQL query: " + ex.getMessage(), ex);
			else if (ex instanceof TranslationException)
				throw new DBException("Unexpected error while translating ADQL into SQL: " + ex.getMessage(), ex);
			else if (ex instanceof DataReadException)
				throw new DBException("Impossible to read the query result, because: " + ex.getMessage(), ex);
			else
				throw new DBException("Unexpected error while executing an ADQL query: " + ex.getMessage(), ex);
		}
	}

	/**
	 * <p>Create a {@link TableIterator} instance which lets reading the given result table.</p>
	 * 
	 * <p><b>Important note 1:</b>
	 * <p><b>Important note:</b>
	 * 	This function also set to NULL the statement of this {@link JDBCConnection} instance: {@link #stmt}.
	 * 	However, the statement is not closed ; it is just given to a {@link ResultSetTableIterator} iterator
	 * 	which will close it in the same time as the given {@link ResultSet}, when its function
	 * 	{@link ResultSetTableIterator#close()} is called.
	 * </p>
	 * 
	 * <p><b>Important note 2:</b>
	 * 	In case an exception occurs within this function, the {@link ResultSet} and the {@link Statement}
	 * 	are <b>immediately closed</b> before propagating the exception.
	 * </p>
	 * 
	 * @param rs				Result of an SQL query.
	 * @param resultingColumns	Metadata corresponding to each columns of the result.
	 * 
@@ -752,10 +754,6 @@ public class JDBCConnection implements DBConnection {
		try{
			return new ResultSetTableIterator(itStmt, rs, translator, dbms, resultingColumns);
		}catch(Throwable t){
			// In case of any kind of exception, the ResultSet and the Statement MUST be closed in order to save resources:
			close(rs);
			close(itStmt);
			// Then, the caught exception can be thrown:
			throw (t instanceof DataReadException) ? (DataReadException)t : new DataReadException(t);
		}
	}
@@ -854,6 +852,7 @@ public class JDBCConnection implements DBConnection {
				logger.logDB(LogLevel.ERROR, this, "LOAD_TAP_SCHEMA", "Impossible to create a Statement!", se);
			throw new DBException("Can not create a Statement!", se);
		}finally{
			cancel(true);
			closeStatement();
		}

@@ -894,7 +893,9 @@ public class JDBCConnection implements DBConnection {

			// Create all schemas:
			while(rs.next()){
				String schemaName = rs.getString(1), description = rs.getString(2), utype = rs.getString(3), dbName = (hasDBName ? rs.getString(4) : null);
				String schemaName = rs.getString(1),
						description = rs.getString(2), utype = rs.getString(3),
						dbName = (hasDBName ? rs.getString(4) : null);

				// create the new schema:
				TAPSchema newSchema = new TAPSchema(schemaName, nullifyIfNeeded(description), nullifyIfNeeded(utype));
@@ -957,7 +958,10 @@ public class JDBCConnection implements DBConnection {
			// Create all tables:
			ArrayList<TAPTable> lstTables = new ArrayList<TAPTable>();
			while(rs.next()){
				String schemaName = rs.getString(1), tableName = rs.getString(2), typeStr = rs.getString(3), description = rs.getString(4), utype = rs.getString(5), dbName = (hasDBName ? rs.getString(6) : null);
				String schemaName = rs.getString(1),
						tableName = rs.getString(2), typeStr = rs.getString(3),
						description = rs.getString(4), utype = rs.getString(5),
						dbName = (hasDBName ? rs.getString(6) : null);

				// get the schema:
				TAPSchema schema = metadata.getSchema(schemaName);
@@ -1049,9 +1053,16 @@ public class JDBCConnection implements DBConnection {

			// Create all tables:
			while(rs.next()){
				String tableName = rs.getString(1), columnName = rs.getString(2), description = rs.getString(3), unit = rs.getString(4), ucd = rs.getString(5), utype = rs.getString(6), datatype = rs.getString(7), dbName = (hasDBName ? rs.getString(12) : null);
				String tableName = rs.getString(1),
						columnName = rs.getString(2),
						description = rs.getString(3), unit = rs.getString(4),
						ucd = rs.getString(5), utype = rs.getString(6),
						datatype = rs.getString(7),
						dbName = (hasDBName ? rs.getString(12) : null);
				int size = rs.getInt(8);
				boolean principal = toBoolean(rs.getObject(9)), indexed = toBoolean(rs.getObject(10)), std = toBoolean(rs.getObject(11));
				boolean principal = toBoolean(rs.getObject(9)),
						indexed = toBoolean(rs.getObject(10)),
						std = toBoolean(rs.getObject(11));

				// get the table:
				TAPTable table = searchTable(tableName, lstTables.iterator());
@@ -1136,7 +1147,9 @@ public class JDBCConnection implements DBConnection {

			// Create all foreign keys:
			while(rs.next()){
				String key_id = rs.getString(1), from_table = rs.getString(2), target_table = rs.getString(3), description = rs.getString(4), utype = rs.getString(5);
				String key_id = rs.getString(1), from_table = rs.getString(2),
						target_table = rs.getString(3),
						description = rs.getString(4), utype = rs.getString(5);

				// get the two tables (source and target):
				TAPTable sourceTable = searchTable(from_table, lstTables.iterator());
@@ -1415,7 +1428,8 @@ public class JDBCConnection implements DBConnection {
			// Identify which standard TAP tables must be dropped:
			rs = dbMeta.getTables(null, null, null, null);
			while(rs.next()){
				String rsSchema = nullifyIfNeeded(rs.getString(2)), rsTable = rs.getString(3);
				String rsSchema = nullifyIfNeeded(rs.getString(2)),
						rsTable = rs.getString(3);
				if (!supportsSchema || (tapSchemaName == null && rsSchema == null) || equals(rsSchema, tapSchemaName, schemaCaseSensitive)){
					int indStdTable;
					indStdTable = getCreationOrder(isStdTable(rsTable, stdTables, tableCaseSensitive));
@@ -2170,6 +2184,7 @@ public class JDBCConnection implements DBConnection {
				logger.logDB(LogLevel.WARNING, this, "DROP_UPLOAD_TABLE", "Impossible to drop the uploaded table: " + translator.getTableName(tableDef, supportsSchema) + "!", se);
			throw new DBException("Impossible to drop the uploaded table: " + translator.getTableName(tableDef, supportsSchema) + "!", se);
		}finally{
			cancel(true);
			closeStatement();
		}
	}