From 9152a622405a62ce3694beb0a4e57dd7c5bf0d68 Mon Sep 17 00:00:00 2001 From: lokiore Date: Wed, 22 Jul 2026 09:12:00 -0700 Subject: [PATCH] PHOENIX-7965 :- Guard ConnectionQueryServicesImpl.getMetaDataCache() against a closed connection getMetaDataCache() returned the volatile latestMetaData field directly, with no null guard. When a connection is closed concurrently with query compilation (for example connection pool teardown or cluster failover), close() sets latestMetaData = null. An in-flight compile then reaches the cache via PhoenixConnection.getTableRef()/getFunction()/getSchema(), which call getMetaDataCache().getTableRef(key) (and friends) on the null return, producing a bare, causeless NullPointerException with no message. Every other cache accessor/mutator on this class (addTable, removeTable, pruneTables, the internal getTable path, etc.) already calls throwConnectionClosedIfNullMetaData() first, which throws a descriptive IllegalStateException("Connection to the cluster is closed"). getMetaDataCache() was the one accessor missing that guard. This change adds the same guard so a closed connection surfaces the descriptive exception instead of a bare NPE. No caller relies on a null return: every production caller of ConnectionQueryServices.getMetaDataCache() immediately dereferences the result, so this only upgrades the failure mode from an opaque NPE to a descriptive exception; it does not change behavior on an open connection. Tests: added unit tests asserting getMetaDataCache() throws the descriptive IllegalStateException when the cache has been nulled (closed-connection state), and returns the cache unchanged when present. Generated-by: Claude Code (Opus 4.8 (1M context)) --- .../query/ConnectionQueryServicesImpl.java | 11 +++- .../ConnectionQueryServicesImplTest.java | 54 +++++++++++++++++++ 2 files changed, 64 insertions(+), 1 deletion(-) diff --git a/phoenix-core-client/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java b/phoenix-core-client/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java index 2c27a57fd37..f14ae7aa711 100644 --- a/phoenix-core-client/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java +++ b/phoenix-core-client/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java @@ -1090,7 +1090,16 @@ private static void throwErrorIfQueryTimedOut(byte[] startRowKey, byte[] endRowK } public PMetaData getMetaDataCache() { - return latestMetaData; + // A concurrent close() (e.g. connection pool teardown or cluster failover) nulls out + // latestMetaData. Read it once into a local so the null-check and the returned reference + // are guaranteed to be the same value; a caller that immediately dereferences the result + // then surfaces the descriptive "connection closed" exception instead of a bare NPE, + // matching every other cache accessor/mutator on this class. + PMetaData cache = latestMetaData; + if (cache == null) { + throwConnectionClosedException(); + } + return cache; } @Override diff --git a/phoenix-core/src/test/java/org/apache/phoenix/query/ConnectionQueryServicesImplTest.java b/phoenix-core/src/test/java/org/apache/phoenix/query/ConnectionQueryServicesImplTest.java index a6a55a5081b..af03a2b0f4e 100644 --- a/phoenix-core/src/test/java/org/apache/phoenix/query/ConnectionQueryServicesImplTest.java +++ b/phoenix-core/src/test/java/org/apache/phoenix/query/ConnectionQueryServicesImplTest.java @@ -73,6 +73,7 @@ import org.apache.phoenix.jdbc.ConnectionInfo; import org.apache.phoenix.jdbc.PhoenixDatabaseMetaData; import org.apache.phoenix.monitoring.GlobalClientMetrics; +import org.apache.phoenix.schema.PMetaData; import org.apache.phoenix.util.ReadOnlyProps; import org.junit.Before; import org.junit.ClassRule; @@ -408,4 +409,57 @@ public void testDropTablesTableEnabled() throws Exception { verify(mockAdmin, Mockito.times(1)).deleteTable(TableName.valueOf("TEST_TABLE")); verify(mockConn).getAdmin(); } + + /** + * When a connection is closed concurrently with query compilation (e.g. connection pool teardown + * or cluster failover), the metadata cache is nulled out. getMetaDataCache() must surface the + * descriptive "connection closed" exception rather than returning null and letting callers hit a + * bare NPE. + */ + @Test + public void testGetMetaDataCacheThrowsWhenClosed() + throws NoSuchFieldException, IllegalAccessException { + ConnectionQueryServicesImpl cqsi = newRealCqsi(); + // Simulate the post-close() state where close() has set latestMetaData = null. + setLatestMetaData(cqsi, null); + try { + cqsi.getMetaDataCache(); + fail("Expected IllegalStateException for a closed connection, but no exception was thrown"); + } catch (IllegalStateException e) { + assertEquals("Connection to the cluster is closed", e.getMessage()); + } + } + + /** + * When the metadata cache is present (open connection), getMetaDataCache() returns it unchanged. + */ + @Test + public void testGetMetaDataCacheReturnsCacheWhenOpen() + throws NoSuchFieldException, IllegalAccessException { + ConnectionQueryServicesImpl cqsi = newRealCqsi(); + PMetaData metaData = Mockito.mock(PMetaData.class); + setLatestMetaData(cqsi, metaData); + assertSame(metaData, cqsi.getMetaDataCache()); + } + + /** + * Builds a real ConnectionQueryServicesImpl so that getMetaDataCache() and its private + * throwConnectionClosedIfNullMetaData() guard execute for real. A CALLS_REAL_METHODS mock cannot + * be used here because Mockito does not route the internal call to the private guard method. + */ + private static ConnectionQueryServicesImpl newRealCqsi() { + QueryServices mockQueryServices = Mockito.mock(QueryServices.class); + ReadOnlyProps emptyProps = ReadOnlyProps.EMPTY_PROPS; + when(mockQueryServices.getProps()).thenReturn(emptyProps); + ConnectionInfo mockConnectionInfo = Mockito.mock(ConnectionInfo.class); + when(mockConnectionInfo.asProps()).thenReturn(emptyProps); + return new ConnectionQueryServicesImpl(mockQueryServices, mockConnectionInfo, new Properties()); + } + + private static void setLatestMetaData(ConnectionQueryServicesImpl cqsi, PMetaData value) + throws NoSuchFieldException, IllegalAccessException { + Field field = ConnectionQueryServicesImpl.class.getDeclaredField("latestMetaData"); + field.setAccessible(true); + field.set(cqsi, value); + } }