Skip to content

Fix exception handling in GraphOrchestrator#afterExecute method to show the exception coming from the TestNGFutureTask. #3248

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
lkalina-cera wants to merge 1 commit into
base: master
Choose a base branch
from
+5 −1
Open

Fix exception handling in GraphOrchestrator#afterExecute method to show the exception coming from the TestNGFutureTask. #3248

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion testng-core/src/main/java/org/testng/internal/thread/graph/GraphOrchestrator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,11 @@

private void afterExecute(IWorker<T> r, Throwable t) {
try (AutoCloseableLock ignore = internalLock.lock()) {
setStatus(r, computeStatus(r));
if (t != null){

Check failure on line 78 in testng-core/src/main/java/org/testng/internal/thread/graph/GraphOrchestrator.java

View workflow job for this annotation

GitHub Actions / 21, microsoft, ubuntu, America/New_York, fr_FR 

Replace 1 line 78..78 with
      if (t != null) {

Check failure on line 78 in testng-core/src/main/java/org/testng/internal/thread/graph/GraphOrchestrator.java

View workflow job for this annotation

GitHub Actions / 17, corretto, ubuntu, UTC, fr_FR, stress JIT 

Replace 1 line 78..78 with
      if (t != null) {

Check failure on line 78 in testng-core/src/main/java/org/testng/internal/thread/graph/GraphOrchestrator.java

View workflow job for this annotation

GitHub Actions / 11, temurin, ubuntu, UTC, tr_TR, stress JIT 

Replace 1 line 78..78 with
      if (t != null) {

Check failure on line 78 in testng-core/src/main/java/org/testng/internal/thread/graph/GraphOrchestrator.java

View workflow job for this annotation

GitHub Actions / 17, liberica, macos, America/New_York, ru_RU 

Replace 1 line 78..78 with
      if (t != null) {

Check failure on line 78 in testng-core/src/main/java/org/testng/internal/thread/graph/GraphOrchestrator.java

View workflow job for this annotation

GitHub Actions / 17, zulu, macos, America/New_York, ru_RU, stress JIT 

Replace 1 line 78..78 with
      if (t != null) {

Check failure on line 78 in testng-core/src/main/java/org/testng/internal/thread/graph/GraphOrchestrator.java

View workflow job for this annotation

GitHub Actions / 21, liberica, macos, UTC, tr_TR, stress JIT 

Replace 1 line 78..78 with
      if (t != null) {
Logger.getLogger(GraphOrchestrator.class).error(t.getMessage(), t);
} else {
setStatus(r, computeStatus(r));
}
Comment on lines +78 to +82
Copy link
Copy Markdown

@coderabbitai coderabbitai bot Nov 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

Critical: Worker status not updated when exception occurs, potentially causing orchestrator to hang.

When an exception is thrown (t != null), the worker's status is never updated from RUNNING. This creates a critical issue:

  1. The shutdown check on line 83 counts only nodes with FINISHED status
  2. If a node fails and remains in RUNNING status, the condition graph.getNodeCount() == graph.getNodeCountWithStatus(IDynamicGraph.Status.FINISHED) may never be satisfied
  3. This prevents service.shutdown() from being called, potentially causing the orchestrator to hang indefinitely

The logging addition successfully addresses the PR objective to show exceptions from TestNGFutureTask. However, the status must still be updated even when exceptions occur.

Apply this diff to update the status even when an exception occurs:

-    if (t != null){
-      Logger.getLogger(GraphOrchestrator.class).error(t.getMessage(), t);
-    } else {
-      setStatus(r, computeStatus(r));
-    }
+    if (t != null) {
+      Logger.getLogger(GraphOrchestrator.class).error(t.getMessage(), t);
+    }
+    setStatus(r, computeStatus(r));

Alternatively, if IDynamicGraph.Status has a FAILED status, you could set nodes to FAILED on exception and update the shutdown logic to account for both FINISHED and FAILED nodes. Please verify the intended behavior when a worker throws an exception:

🏁 Script executed:

#!/bin/bash
# Description: Check if IDynamicGraph.Status has a FAILED status and how shutdown should behave

# Search for Status enum definition
ast-grep --pattern 'enum Status {
  $$$
}'

# Search for how getNodeCountWithStatus is used elsewhere
rg -n -C3 'getNodeCountWithStatus'

# Search for other exception handling patterns in the codebase
rg -n -C3 'afterExecute.*Throwable'

Length of output: 4415

🏁 Script executed:

#!/bin/bash
# Check computeStatus implementation and setStatus behavior

# Find computeStatus method
ast-grep --pattern 'private $_ computeStatus($_) {
  $$$
}'

# Find setStatus method
rg -n -C5 'private void setStatus'

# Check if there's any other shutdown mechanism or exception recovery
rg -n 'service.shutdown' 

# Check TestNGFutureTask to understand exception handling context
rg -n 'class TestNGFutureTask' -A 20

Length of output: 4811

🏁 Script executed:

#!/bin/bash
# Verify computeStatus behavior and status update mechanisms

# Find computeStatus implementation
ast-grep --pattern 'private $_ computeStatus($_) {
  $$$
}'

# Find setStatus implementation  
rg -n 'private void setStatus' -A 5

# Check if there's any other place that marks nodes as FINISHED besides afterExecute
rg -n 'setStatus.*FINISHED'

# Check the complete shutdown logic
rg -n 'service.shutdown' -B 5 -A 2

# Look for exception handling or finally blocks in afterExecute context
rg -n 'afterExecute' -A 20

Length of output: 12096

Critical: Worker status not updated when exception occurs, preventing orchestrator shutdown and causing potential hang.

When an exception is thrown (t != null), the setStatus() method is never called. This creates a critical reliability issue:

  1. The worker's tasks remain in RUNNING status (never transitioned to FINISHED or READY)
  2. The shutdown check on line 83 requires all nodes to have FINISHED status
  3. If any node fails with an exception and stays in RUNNING, the condition is never satisfied
  4. The service.shutdown() is never invoked, leaving the thread pool active and potentially causing the orchestrator to hang indefinitely

The logging addition successfully achieves the PR objective to expose exceptions from TestNGFutureTask. However, status management must be updated regardless of exception state to maintain the shutdown contract.

Fix: Move setStatus() outside the conditional so it executes for both exception and success paths:

-    if (t != null){
-      Logger.getLogger(GraphOrchestrator.class).error(t.getMessage(), t);
-    } else {
-      setStatus(r, computeStatus(r));
-    }
+    if (t != null) {
+      Logger.getLogger(GraphOrchestrator.class).error(t.getMessage(), t);
+    }
+    setStatus(r, computeStatus(r));

This ensures all nodes transition to FINISHED (or READY for thread affinity) even when exceptions occur, allowing the shutdown condition to be met.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if (t != null){
Logger.getLogger(GraphOrchestrator.class).error(t.getMessage(), t);
} else {
setStatus(r, computeStatus(r));
}
if (t != null) {
Logger.getLogger(GraphOrchestrator.class).error(t.getMessage(), t);
}
setStatus(r, computeStatus(r));
🤖 Prompt for AI Agents 
In
testng-core/src/main/java/org/testng/internal/thread/graph/GraphOrchestrator.java
around lines 78 to 82, the code logs the exception but fails to update the
worker node status when t != null, leaving nodes in RUNNING and preventing
shutdown; move the call to setStatus(r, computeStatus(r)) out of the if/else so
it executes regardless of whether an exception occurred (keep the
Logger.error(t.getMessage(), t) in the t != null branch), ensuring nodes
transition to FINISHED (or READY for thread affinity) even on exceptions so the
shutdown condition can be met.

if (graph.getNodeCount() == graph.getNodeCountWithStatus(IDynamicGraph.Status.FINISHED)) {
service.shutdown();
} else {
Expand Down
Loading