refactor: Eliminate nested exception handling and reduce control flow complexity

p123-stack · p123-stack · commit c5de06c101ea · 2026-01-23T12:18:11.000+05:30
diff --git a/src/Bolt/BoltConnection.php b/src/Bolt/BoltConnection.php
@@ -326,7 +326,11 @@ public function close(): void
 
                 unset($this->boltProtocol); // has to be set to null as the sockets don't recover nicely contrary to what the underlying code might lead you to believe;
             }
-        } catch (Throwable) {
+        } catch (Throwable $e) {
+            $this->logger?->log(LogLevel::WARNING, 'Failed to close connection gracefully', [
+                'exception' => $e->getMessage(),
+                'type' => $e::class,
+            ]);
         }
     }
 
@@ -341,25 +345,17 @@ public function close(): void
      */
     public function invalidate(): void
     {
+        $this->subscribedResults = [];
         try {
-            $this->subscribedResults = [];
-
-            try {
-                $this->connection->disconnect();
-            } catch (Throwable $e) {
-                $this->logger?->log(LogLevel::WARNING, 'Failed to disconnect during invalidation', [
-                    'exception' => $e->getMessage(),
-                    'type' => $e::class,
-                ]);
-            }
-
-            unset($this->boltProtocol);
+            $this->connection->disconnect();
         } catch (Throwable $e) {
-            $this->logger?->log(LogLevel::WARNING, 'Error during connection invalidation', [
+            $this->logger?->log(LogLevel::WARNING, 'Failed to disconnect during invalidation', [
                 'exception' => $e->getMessage(),
                 'type' => $e::class,
             ]);
         }
+
+        unset($this->boltProtocol);
     }
 
     private function buildRunExtra(?string $database, ?float $timeout, ?BookmarkHolder $holder, ?AccessMode $mode, ?iterable $metadata): array
diff --git a/src/Bolt/Session.php b/src/Bolt/Session.php
@@ -23,6 +23,7 @@
 use Laudis\Neo4j\Contracts\UnmanagedTransactionInterface;
 use Laudis\Neo4j\Databags\Bookmark;
 use Laudis\Neo4j\Databags\BookmarkHolder;
+use Laudis\Neo4j\Databags\Neo4jError;
 use Laudis\Neo4j\Databags\SessionConfiguration;
 use Laudis\Neo4j\Databags\Statement;
 use Laudis\Neo4j\Databags\SummarizedResult;
@@ -76,54 +77,7 @@ public function runStatements(iterable $statements, ?TransactionConfiguration $c
         $config = $this->mergeTsxConfig($config);
 
         foreach ($statements as $statement) {
-            // Wrap in retry logic for connection errors
-            $retries = 0;
-            $maxRetries = 3;
-
-            while ($retries < $maxRetries) {
-                try {
-                    $tbr[] = $this->beginInstantTransaction($this->config, $config)->runStatement($statement);
-                    break; // Success, exit retry loop
-                } catch (Neo4jException $e) {
-                    if ($this->shouldClearRoutingTable($e)) {
-                        $this->getLogger()?->log(LogLevel::WARNING, 'Connection error in instant transaction, retrying', [
-                            'error' => $e->getMessage(),
-                            'retry' => $retries + 1,
-                        ]);
-
-                        if ($this->pool instanceof Neo4jConnectionPool) {
-                            $this->pool->clearRoutingTable($this->config);
-                        }
-                        $this->pool->close();
-
-                        ++$retries;
-                        if ($retries >= $maxRetries) {
-                            throw $e;
-                        }
-                    } else {
-                        throw $e;
-                    }
-                } catch (Throwable $e) {
-                    if ($this->isConnectionError($e)) {
-                        $this->getLogger()?->log(LogLevel::WARNING, 'Connection error in instant transaction, retrying', [
-                            'error' => $e->getMessage(),
-                            'retry' => $retries + 1,
-                        ]);
-
-                        if ($this->pool instanceof Neo4jConnectionPool) {
-                            $this->pool->clearRoutingTable($this->config);
-                        }
-                        $this->pool->close();
-
-                        ++$retries;
-                        if ($retries >= $maxRetries) {
-                            throw $e;
-                        }
-                    } else {
-                        throw $e;
-                    }
-                }
-            }
+            $tbr[] = $this->executeStatementWithRetry($statement, $config);
         }
 
         return new CypherList($tbr);
@@ -186,44 +140,73 @@ private function retry(callable $tsxHandler, bool $read, TransactionConfiguratio
 
                 return $tbr;
             } catch (Neo4jException $e) {
-                if ($transaction && !in_array($e->getClassification(), self::ROLLBACK_CLASSIFICATIONS)) {
-                    $transaction->rollback();
-                }
-
-                // ADD THIS SECTION - Handle connection timeouts and routing failures
-                if ($e->getTitle() === 'NotALeader'
-                    || $e->getNeo4jCode() === 'Neo.ClientError.Cluster.NotALeader'
-                    || $this->isConnectionError($e)) {
-                    // Clear routing table before closing pool to force fresh ROUTE request on retry
-                    if ($this->pool instanceof Neo4jConnectionPool) {
-                        $this->pool->clearRoutingTable($this->config);
-                    }
-                    // By closing the pool, we force the connection to be re-acquired and the routing table to be refetched
-                    $this->pool->close();
-                } elseif ($e->getClassification() !== 'TransientError') {
-                    throw $e;
-                }
-            } catch (Exception $e) {
-                if ($this->isConnectionError($e)) {
-                    if ($this->pool instanceof Neo4jConnectionPool) {
-                        $this->pool->clearRoutingTable($this->config);
-                    }
-                    $this->pool->close();
-                } else {
+                $this->handleManagedTransactionError($transaction, $e);
+            } catch (Throwable $e) {
+                // For non-Neo4jException errors, only retry on connection errors
+                if (!$this->isConnectionError($e)) {
                     throw $e;
                 }
+                // Connection error - clear routing and retry
+                $this->handleConnectionFailure();
             }
         }
     }
 
+    /**
+     * Handle Neo4jException in managed transaction - either rollback and retry or throw.
+     *
+     * @param Neo4jException $e The exception that occurred
+     */
+    private function handleManagedTransactionError(?UnmanagedTransactionInterface $transaction, Neo4jException $e): void
+    {
+        if ($transaction && !in_array($e->getClassification(), self::ROLLBACK_CLASSIFICATIONS)) {
+            $transaction->rollback();
+        }
+
+        if ($this->shouldRetryManagedTransaction($e)) {
+            $this->handleConnectionFailure();
+
+            return;
+        }
+
+        throw $e;
+    }
+
+    /**
+     * Determine if a Neo4jException should trigger retry of managed transaction.
+     */
+    private function shouldRetryManagedTransaction(Neo4jException $e): bool
+    {
+        if ($e->getTitle() === 'NotALeader' || $e->getNeo4jCode() === 'Neo.ClientError.Cluster.NotALeader') {
+            return true;
+        }
+
+        if ($this->isConnectionError($e)) {
+            return true;
+        }
+
+        return $e->getClassification() === 'TransientError';
+    }
+
+    /**
+     * Handle connection failure by clearing routing table and closing pool.
+     * This forces fresh connection acquisition and routing table refresh on next attempt.
+     */
+    private function handleConnectionFailure(): void
+    {
+        if ($this->pool instanceof Neo4jConnectionPool) {
+            $this->pool->clearRoutingTable($this->config);
+        }
+        $this->pool->close();
+    }
+
     /**
      * Check if the exception is a connection-related error.
      */
     private function isConnectionError(Throwable $e): bool
     {
         $message = strtolower($e->getMessage());
 
-        // Check for common connection error messages
         if (str_contains($message, 'interrupted system call')
             || str_contains($message, 'broken pipe')
             || str_contains($message, 'connection reset')
@@ -232,7 +215,6 @@ private function isConnectionError(Throwable $e): bool
             return true;
         }
 
-        // Check for Neo4jException-specific codes
         if ($e instanceof Neo4jException) {
             return $e->getNeo4jCode() === 'Neo.ClientError.Cluster.NotALeader';
         }
@@ -248,7 +230,6 @@ private function shouldClearRoutingTable(Neo4jException $e): bool
         $message = strtolower($e->getMessage());
         $title = $e->getTitle();
 
-        // Clear routing table for timeout, connection, and cluster errors
         return str_contains($message, 'interrupted system call')
             || str_contains($message, 'broken pipe')
             || str_contains($message, 'connection reset')
@@ -258,6 +239,58 @@ private function shouldClearRoutingTable(Neo4jException $e): bool
             || $title === 'NotALeader';
     }
 
+    /**
+     * Execute a statement with automatic retry on connection errors.
+     * Retries up to 3 times on connection failures, clearing routing table between attempts.
+     *
+     * @param Statement                $statement The statement to execute
+     * @param TransactionConfiguration $config    Transaction configuration
+     *
+     * @return SummarizedResult The result of the statement
+     */
+    private function executeStatementWithRetry(Statement $statement, TransactionConfiguration $config): SummarizedResult
+    {
+        $maxRetries = 3;
+        $retries = 0;
+
+        while ($retries < $maxRetries) {
+            try {
+                return $this->beginInstantTransaction($this->config, $config)->runStatement($statement);
+            } catch (Neo4jException $e) {
+                if (!$this->shouldClearRoutingTable($e)) {
+                    throw $e;
+                }
+                $this->handleStatementRetry($retries, $maxRetries, $e);
+            } catch (Throwable $e) {
+                if (!$this->isConnectionError($e)) {
+                    throw $e;
+                }
+                $this->handleStatementRetry($retries, $maxRetries, $e);
+            }
+        }
+
+        throw new Neo4jException([Neo4jError::fromMessageAndCode('Neo.ClientError.General', 'Statement execution failed after maximum retries')]);
+    }
+
+    /**
+     * Handle retry logic for statement execution - clear routing and increment counter.
+     * Throws the exception if max retries exceeded.
+     */
+    private function handleStatementRetry(int &$retries, int $maxRetries, Throwable $e): void
+    {
+        $this->getLogger()?->log(LogLevel::WARNING, 'Connection error in instant transaction, retrying', [
+            'error' => $e->getMessage(),
+            'retry' => $retries + 1,
+        ]);
+
+        $this->handleConnectionFailure();
+
+        ++$retries;
+        if ($retries >= $maxRetries) {
+            throw $e;
+        }
+    }
+
     private static function triggerLazyResult(mixed $tbr): void
     {
         if ($tbr instanceof CypherSequence) {
@@ -339,18 +372,13 @@ private function acquireConnection(TransactionConfiguration $config, SessionConf
     private function startTransaction(TransactionConfiguration $config, SessionConfiguration $sessionConfig): UnmanagedTransactionInterface
     {
         $this->getLogger()?->log(LogLevel::INFO, 'Starting transaction', ['config' => $config, 'sessionConfig' => $sessionConfig]);
-        try {
-            $connection = $this->acquireConnection($config, $sessionConfig);
+        $connection = $this->acquireConnection($config, $sessionConfig);
 
+        try {
             $connection->begin($this->config->getDatabase(), $config->getTimeout(), $this->bookmarkHolder, $config->getMetaData());
         } catch (Neo4jException $e) {
-            if (isset($connection) && $connection->getServerState() === 'FAILED') {
-                $connection->reset();
-            }
-            // Release connection back to pool if available
-            if (isset($connection)) {
-                $this->pool->release($connection);
-            }
+            // BEGIN failed - clean up connection before rethrowing
+            $this->cleanupFailedConnection($connection);
             throw $e;
         }
 
@@ -370,6 +398,19 @@ private function startTransaction(TransactionConfiguration $config, SessionConfi
         );
     }
 
+    /**
+     * Clean up a connection that failed during BEGIN or other initialization.
+     * Resets the connection if it's in FAILED state and releases it back to the pool.
+     */
+    private function cleanupFailedConnection(BoltConnection $connection): void
+    {
+        if ($connection->getServerState() === 'FAILED') {
+            $connection->reset();
+        }
+        // Release connection back to pool for reuse
+        $this->pool->release($connection);
+    }
+
     private function mergeTsxConfig(?TransactionConfiguration $config): TransactionConfiguration
     {
         return TransactionConfiguration::default()->merge($config);
diff --git a/src/Contracts/BoltMessage.php b/src/Contracts/BoltMessage.php
@@ -42,18 +42,16 @@ public function getResponse(): Response
                 if (preg_match('/(\d+)\s*(?:milliseconds?|ms|seconds?|s)/', $e->getMessage(), $matches) && array_key_exists(1, $matches)) {
                     $timeoutMsg = 'Connection timeout reached after '.$matches[1].' seconds';
                 }
+
                 try {
-                    // Use invalidate() instead of close() to avoid sending GOODBYE on timeout
                     $this->connection->invalidate();
-                } catch (Throwable) {
+                } catch (Throwable $invalidateException) {
                 }
-                // Use DriverError so the driver treats this as a failure
                 throw new Neo4jException([Neo4jError::fromMessageAndCode('Neo.ClientError.Cluster.NotALeader', $timeoutMsg)], $e);
             } elseif ($this->isSocketException($e)) {
                 try {
-                    // Use invalidate() instead of close() to avoid sending GOODBYE on socket errors
                     $this->connection->invalidate();
-                } catch (Throwable) {
+                } catch (Throwable $invalidateException) {
                 }
                 throw new Neo4jException([Neo4jError::fromMessageAndCode('Neo.ClientError.Cluster.NotALeader', 'Connection error: '.$e->getMessage())], $e);
             }

Original file line number	Diff line number	Diff line change
`@@ -42,18 +42,16 @@ public function getResponse(): Response`
`42`	`42`	`if (preg_match('/(\d+)\s*(?:milliseconds?\|ms\|seconds?\|s)/', $e->getMessage(), $matches) && array_key_exists(1, $matches)) {`
`43`	`43`	`$timeoutMsg = 'Connection timeout reached after '.$matches[1].' seconds';`
`44`	`44`	`}`
	`45`	`+`
`45`	`46`	`try {`
`46`		`- // Use invalidate() instead of close() to avoid sending GOODBYE on timeout`
`47`	`47`	`$this->connection->invalidate();`
`48`		`- } catch (Throwable) {`
	`48`	`+ } catch (Throwable $invalidateException) {`
`49`	`49`	`}`
`50`		`- // Use DriverError so the driver treats this as a failure`
`51`	`50`	`throw new Neo4jException([Neo4jError::fromMessageAndCode('Neo.ClientError.Cluster.NotALeader', $timeoutMsg)], $e);`
`52`	`51`	`} elseif ($this->isSocketException($e)) {`
`53`	`52`	`try {`
`54`		`- // Use invalidate() instead of close() to avoid sending GOODBYE on socket errors`
`55`	`53`	`$this->connection->invalidate();`
`56`		`- } catch (Throwable) {`
	`54`	`+ } catch (Throwable $invalidateException) {`
`57`	`55`	`}`
`58`	`56`	`throw new Neo4jException([Neo4jError::fromMessageAndCode('Neo.ClientError.Cluster.NotALeader', 'Connection error: '.$e->getMessage())], $e);`
`59`	`57`	`}`