Return-Path: X-Original-To: apmail-phoenix-commits-archive@minotaur.apache.org Delivered-To: apmail-phoenix-commits-archive@minotaur.apache.org Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by minotaur.apache.org (Postfix) with SMTP id 84F561890C for ; Thu, 21 Jan 2016 01:07:55 +0000 (UTC) Received: (qmail 42065 invoked by uid 500); 21 Jan 2016 01:07:55 -0000 Delivered-To: apmail-phoenix-commits-archive@phoenix.apache.org Received: (qmail 42027 invoked by uid 500); 21 Jan 2016 01:07:55 -0000 Mailing-List: contact commits-help@phoenix.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: dev@phoenix.apache.org Delivered-To: mailing list commits@phoenix.apache.org Received: (qmail 42018 invoked by uid 99); 21 Jan 2016 01:07:55 -0000 Received: from git1-us-west.apache.org (HELO git1-us-west.apache.org) (140.211.11.23) by apache.org (qpsmtpd/0.29) with ESMTP; Thu, 21 Jan 2016 01:07:55 +0000 Received: by git1-us-west.apache.org (ASF Mail Server at git1-us-west.apache.org, from userid 33) id 4EC12DFF93; Thu, 21 Jan 2016 01:07:55 +0000 (UTC) Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit From: samarth@apache.org To: commits@phoenix.apache.org Message-Id: X-Mailer: ASF-Git Admin Mailer Subject: phoenix git commit: PHOENIX-2589 Fix a few resource leaks, NULL dereference, NULL_RETURNS issues (Samarth Jain, Alicia Ying Shu) Date: Thu, 21 Jan 2016 01:07:55 +0000 (UTC) Repository: phoenix Updated Branches: refs/heads/4.x-HBase-1.0 84e0d8997 -> 15368f501 PHOENIX-2589 Fix a few resource leaks, NULL dereference, NULL_RETURNS issues (Samarth Jain, Alicia Ying Shu) Project: http://git-wip-us.apache.org/repos/asf/phoenix/repo Commit: http://git-wip-us.apache.org/repos/asf/phoenix/commit/15368f50 Tree: http://git-wip-us.apache.org/repos/asf/phoenix/tree/15368f50 Diff: http://git-wip-us.apache.org/repos/asf/phoenix/diff/15368f50 Branch: refs/heads/4.x-HBase-1.0 Commit: 15368f5018b66032cddeedb41e925b9acc86b363 Parents: 84e0d89 Author: Samarth Authored: Wed Jan 20 17:07:49 2016 -0800 Committer: Samarth Committed: Wed Jan 20 17:07:49 2016 -0800 ---------------------------------------------------------------------- .../query/ConnectionQueryServicesImpl.java | 20 +- .../apache/phoenix/schema/MetaDataClient.java | 235 ++++++++++--------- pom.xml | 1 + 3 files changed, 137 insertions(+), 119 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/phoenix/blob/15368f50/phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java ---------------------------------------------------------------------- diff --git a/phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java b/phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java index fe7466f..f858a5c 100644 --- a/phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java +++ b/phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java @@ -3311,8 +3311,14 @@ public class ConnectionQueryServicesImpl extends DelegateQueryServices implement wait = false; } // It is guaranteed that this poll won't hang indefinitely because this is the - // only thread that removes items from the queue. - WeakReference connRef = connectionsQueue.poll(); + // only thread that removes items from the queue. Still adding a 1 ms timeout + // for sanity check. + WeakReference connRef = + connectionsQueue.poll(1, TimeUnit.MILLISECONDS); + if (connRef == null) { + throw new IllegalStateException( + "Connection ref found to be null. This is a bug. Some other thread removed items from the connection queue."); + } PhoenixConnection conn = connRef.get(); if (conn != null && !conn.isClosed()) { LinkedBlockingQueue> scannerQueue = @@ -3323,7 +3329,15 @@ public class ConnectionQueryServicesImpl extends DelegateQueryServices implement int renewed = 0; long start = System.currentTimeMillis(); while (numScanners > 0) { - WeakReference ref = scannerQueue.poll(); + // It is guaranteed that this poll won't hang indefinitely because this is the + // only thread that removes items from the queue. Still adding a 1 ms timeout + // for sanity check. + WeakReference ref = + scannerQueue.poll(1, TimeUnit.MILLISECONDS); + if (ref == null) { + throw new IllegalStateException( + "TableResulIterator ref found to be null. This is a bug. Some other thread removed items from the scanner queue."); + } TableResultIterator scanningItr = ref.get(); if (scanningItr != null) { RenewLeaseStatus status = scanningItr.renewLease(); http://git-wip-us.apache.org/repos/asf/phoenix/blob/15368f50/phoenix-core/src/main/java/org/apache/phoenix/schema/MetaDataClient.java ---------------------------------------------------------------------- diff --git a/phoenix-core/src/main/java/org/apache/phoenix/schema/MetaDataClient.java b/phoenix-core/src/main/java/org/apache/phoenix/schema/MetaDataClient.java index 0b446b3..064007f 100644 --- a/phoenix-core/src/main/java/org/apache/phoenix/schema/MetaDataClient.java +++ b/phoenix-core/src/main/java/org/apache/phoenix/schema/MetaDataClient.java @@ -1450,25 +1450,26 @@ public class MetaDataClient { List functionData = Lists.newArrayListWithExpectedSize(function.getFunctionArguments().size() + 1); List args = function.getFunctionArguments(); - PreparedStatement argUpsert = connection.prepareStatement(INSERT_FUNCTION_ARGUMENT); - - for (int i = 0; i < args.size(); i++) { - FunctionArgument arg = args.get(i); - addFunctionArgMutation(function.getFunctionName(), arg, argUpsert, i); + try (PreparedStatement argUpsert = connection.prepareStatement(INSERT_FUNCTION_ARGUMENT)) { + for (int i = 0; i < args.size(); i++) { + FunctionArgument arg = args.get(i); + addFunctionArgMutation(function.getFunctionName(), arg, argUpsert, i); + } + functionData.addAll(connection.getMutationState().toMutations().next().getSecond()); + connection.rollback(); } - functionData.addAll(connection.getMutationState().toMutations().next().getSecond()); - connection.rollback(); - PreparedStatement functionUpsert = connection.prepareStatement(CREATE_FUNCTION); - functionUpsert.setString(1, tenantIdStr); - functionUpsert.setString(2, function.getFunctionName()); - functionUpsert.setInt(3, function.getFunctionArguments().size()); - functionUpsert.setString(4, function.getClassName()); - functionUpsert.setString(5, function.getJarPath()); - functionUpsert.setString(6, function.getReturnType()); - functionUpsert.execute(); - functionData.addAll(connection.getMutationState().toMutations(null).next().getSecond()); - connection.rollback(); + try (PreparedStatement functionUpsert = connection.prepareStatement(CREATE_FUNCTION)) { + functionUpsert.setString(1, tenantIdStr); + functionUpsert.setString(2, function.getFunctionName()); + functionUpsert.setInt(3, function.getFunctionArguments().size()); + functionUpsert.setString(4, function.getClassName()); + functionUpsert.setString(5, function.getJarPath()); + functionUpsert.setString(6, function.getReturnType()); + functionUpsert.execute(); + functionData.addAll(connection.getMutationState().toMutations(null).next().getSecond()); + connection.rollback(); + } MetaDataMutationResult result = connection.getQueryServices().createFunction(functionData, function, stmt.isTemporary()); MutationCode code = result.getMutationCode(); switch(code) { @@ -1880,7 +1881,6 @@ public class MetaDataClient { } } - PreparedStatement colUpsert = connection.prepareStatement(INSERT_COLUMN_CREATE_TABLE); Map familyNames = Maps.newLinkedHashMap(); boolean isPK = false; boolean rowTimeStampColumnAlreadyFound = false; @@ -2056,38 +2056,40 @@ public class MetaDataClient { } short nextKeySeq = 0; - for (int i = 0; i < columns.size(); i++) { - PColumn column = columns.get(i); - final int columnPosition = column.getPosition(); - // For client-side cache, we need to update the column - if (isViewColumnReferenced != null) { - if (viewColumnConstants != null && columnPosition < viewColumnConstants.length) { - columns.set(i, column = new DelegateColumn(column) { - @Override - public byte[] getViewConstant() { - return viewColumnConstants[columnPosition]; - } - @Override - public boolean isViewReferenced() { - return isViewColumnReferenced.get(columnPosition); - } - }); - } else { - columns.set(i, column = new DelegateColumn(column) { - @Override - public boolean isViewReferenced() { - return isViewColumnReferenced.get(columnPosition); - } - }); + + try (PreparedStatement colUpsert = connection.prepareStatement(INSERT_COLUMN_CREATE_TABLE)) { + for (int i = 0; i < columns.size(); i++) { + PColumn column = columns.get(i); + final int columnPosition = column.getPosition(); + // For client-side cache, we need to update the column + if (isViewColumnReferenced != null) { + if (viewColumnConstants != null && columnPosition < viewColumnConstants.length) { + columns.set(i, column = new DelegateColumn(column) { + @Override + public byte[] getViewConstant() { + return viewColumnConstants[columnPosition]; + } + @Override + public boolean isViewReferenced() { + return isViewColumnReferenced.get(columnPosition); + } + }); + } else { + columns.set(i, column = new DelegateColumn(column) { + @Override + public boolean isViewReferenced() { + return isViewColumnReferenced.get(columnPosition); + } + }); + } } + Short keySeq = SchemaUtil.isPKColumn(column) ? ++nextKeySeq : null; + addColumnMutation(schemaName, tableName, column, colUpsert, parentTableName, pkName, keySeq, saltBucketNum != null); } - Short keySeq = SchemaUtil.isPKColumn(column) ? ++nextKeySeq : null; - addColumnMutation(schemaName, tableName, column, colUpsert, parentTableName, pkName, keySeq, saltBucketNum != null); + tableMetaData.addAll(connection.getMutationState().toMutations(timestamp).next().getSecond()); + connection.rollback(); } - tableMetaData.addAll(connection.getMutationState().toMutations(timestamp).next().getSecond()); - connection.rollback(); - String dataTableName = parent == null || tableType == PTableType.VIEW ? null : parent.getTableName().getString(); PIndexState indexState = parent == null || tableType == PTableType.VIEW ? null : PIndexState.BUILDING; PreparedStatement tableUpsert = connection.prepareStatement(CREATE_TABLE); @@ -2572,12 +2574,13 @@ public class MetaDataClient { TABLE_NAME + "," + propertyName + ") VALUES (?, ?, ?, ?)"; - PreparedStatement tableBoolUpsert = connection.prepareStatement(updatePropertySql); - tableBoolUpsert.setString(1, tenantId); - tableBoolUpsert.setString(2, schemaName); - tableBoolUpsert.setString(3, tableName); - tableBoolUpsert.setBoolean(4, propertyValue); - tableBoolUpsert.execute(); + try (PreparedStatement tableBoolUpsert = connection.prepareStatement(updatePropertySql)) { + tableBoolUpsert.setString(1, tenantId); + tableBoolUpsert.setString(2, schemaName); + tableBoolUpsert.setString(3, tableName); + tableBoolUpsert.setBoolean(4, propertyValue); + tableBoolUpsert.execute(); + } } private void mutateLongProperty(String tenantId, String schemaName, String tableName, @@ -2588,12 +2591,13 @@ public class MetaDataClient { TABLE_NAME + "," + propertyName + ") VALUES (?, ?, ?, ?)"; - PreparedStatement tableBoolUpsert = connection.prepareStatement(updatePropertySql); - tableBoolUpsert.setString(1, tenantId); - tableBoolUpsert.setString(2, schemaName); - tableBoolUpsert.setString(3, tableName); - tableBoolUpsert.setLong(4, propertyValue); - tableBoolUpsert.execute(); + try (PreparedStatement tableBoolUpsert = connection.prepareStatement(updatePropertySql)) { + tableBoolUpsert.setString(1, tenantId); + tableBoolUpsert.setString(2, schemaName); + tableBoolUpsert.setString(3, tableName); + tableBoolUpsert.setLong(4, propertyValue); + tableBoolUpsert.execute(); + } } public MutationState addColumn(AddColumnStatement statement) throws SQLException { @@ -2743,77 +2747,76 @@ public class MetaDataClient { Long timeStamp = TransactionUtil.getTableTimestamp(connection, table.isTransactional() || nonTxToTx); int numPkColumnsAdded = 0; - PreparedStatement colUpsert = connection.prepareStatement(INSERT_COLUMN_ALTER_TABLE); - List columns = Lists.newArrayListWithExpectedSize(columnDefs.size()); Set colFamiliesForPColumnsToBeAdded = new LinkedHashSet<>(); Set families = new LinkedHashSet<>(); if (columnDefs.size() > 0 ) { - short nextKeySeq = SchemaUtil.getMaxKeySeq(table); - for( ColumnDef colDef : columnDefs) { - if (colDef != null && !colDef.isNull()) { - if(colDef.isPK()) { - throw new SQLExceptionInfo.Builder(SQLExceptionCode.NOT_NULLABLE_COLUMN_IN_ROW_KEY) - .setColumnName(colDef.getColumnDefName().getColumnName()).build().buildException(); - } else { - throw new SQLExceptionInfo.Builder(SQLExceptionCode.CANNOT_ADD_NOT_NULLABLE_COLUMN) + try (PreparedStatement colUpsert = connection.prepareStatement(INSERT_COLUMN_ALTER_TABLE)) { + short nextKeySeq = SchemaUtil.getMaxKeySeq(table); + for( ColumnDef colDef : columnDefs) { + if (colDef != null && !colDef.isNull()) { + if(colDef.isPK()) { + throw new SQLExceptionInfo.Builder(SQLExceptionCode.NOT_NULLABLE_COLUMN_IN_ROW_KEY) + .setColumnName(colDef.getColumnDefName().getColumnName()).build().buildException(); + } else { + throw new SQLExceptionInfo.Builder(SQLExceptionCode.CANNOT_ADD_NOT_NULLABLE_COLUMN) + .setColumnName(colDef.getColumnDefName().getColumnName()).build().buildException(); + } + } + if (colDef != null && colDef.isPK() && table.getType() == VIEW && table.getViewType() != MAPPED) { + throwIfLastPKOfParentIsFixedLength(getParentOfView(table), schemaName, tableName, colDef); + } + if (colDef != null && colDef.isRowTimestamp()) { + throw new SQLExceptionInfo.Builder(SQLExceptionCode.ROWTIMESTAMP_CREATE_ONLY) .setColumnName(colDef.getColumnDefName().getColumnName()).build().buildException(); } - } - if (colDef != null && colDef.isPK() && table.getType() == VIEW && table.getViewType() != MAPPED) { - throwIfLastPKOfParentIsFixedLength(getParentOfView(table), schemaName, tableName, colDef); - } - if (colDef != null && colDef.isRowTimestamp()) { - throw new SQLExceptionInfo.Builder(SQLExceptionCode.ROWTIMESTAMP_CREATE_ONLY) - .setColumnName(colDef.getColumnDefName().getColumnName()).build().buildException(); - } - PColumn column = newColumn(position++, colDef, PrimaryKeyConstraint.EMPTY, table.getDefaultFamilyName() == null ? null : table.getDefaultFamilyName().getString(), true); - columns.add(column); - String pkName = null; - Short keySeq = null; - - // TODO: support setting properties on other families? - if (column.getFamilyName() == null) { - ++numPkColumnsAdded; - pkName = table.getPKName() == null ? null : table.getPKName().getString(); - keySeq = ++nextKeySeq; - } else { - families.add(column.getFamilyName().getString()); - } - colFamiliesForPColumnsToBeAdded.add(column.getFamilyName() == null ? null : column.getFamilyName().getString()); - addColumnMutation(schemaName, tableName, column, colUpsert, null, pkName, keySeq, table.getBucketNum() != null); - } - - // Add any new PK columns to end of index PK - if (numPkColumnsAdded>0) { - // create PK column list that includes the newly created columns - List pkColumns = Lists.newArrayListWithExpectedSize(table.getPKColumns().size()+numPkColumnsAdded); - pkColumns.addAll(table.getPKColumns()); - for (int i=0; i0) { + // create PK column list that includes the newly created columns + List pkColumns = Lists.newArrayListWithExpectedSize(table.getPKColumns().size()+numPkColumnsAdded); + pkColumns.addAll(table.getPKColumns()); for (int i=0; i-enableassertions -Xmx2250m -XX:MaxPermSize=128m -Djava.security.egd=file:/dev/./urandom "-Djava.library.path=${hadoop.library.path}${path.separator}${java.library.path}" ${test.output.tofile} + kill