Fix exception handling in GraphOrchestrator#afterExecute method to show the exception coming from the TestNGFutureTask. #3248
Conversation
the exception coming from the TestNGFutureTask.
WalkthroughThe Changes
Sequence Diagram(s)sequenceDiagram
participant Worker
participant GraphOrchestrator
participant Logger
participant NodeScheduler
Worker->>GraphOrchestrator: afterExecute (task completes)
alt Task completed with exception
GraphOrchestrator->>Logger: Log error
Logger-->>GraphOrchestrator: Error recorded
Note over GraphOrchestrator: Skip status update
GraphOrchestrator->>NodeScheduler: No status applied
else Task completed successfully
GraphOrchestrator->>GraphOrchestrator: Compute worker status
Note over GraphOrchestrator: Apply status update
GraphOrchestrator->>NodeScheduler: Schedule next nodes
end
GraphOrchestrator-->>Worker: Handler complete
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
testng-core/src/main/java/org/testng/internal/thread/graph/GraphOrchestrator.java (1)
79-79: Consider using a fallback message whent.getMessage()returns null.The
getMessage()method can return null for someThrowableinstances. While SLF4J handles null messages gracefully, providing a fallback improves log clarity.Apply this diff:
- Logger.getLogger(GraphOrchestrator.class).error(t.getMessage(), t); + String message = t.getMessage() != null ? t.getMessage() : "Worker execution failed"; + Logger.getLogger(GraphOrchestrator.class).error(message, t);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
testng-core/src/main/java/org/testng/internal/thread/graph/GraphOrchestrator.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
testng-core/src/main/java/org/testng/internal/thread/graph/GraphOrchestrator.java (1)
testng-core-api/src/main/java/org/testng/log4testng/Logger.java (1)
Logger(13-192)
| if (t != null){ | ||
| Logger.getLogger(GraphOrchestrator.class).error(t.getMessage(), t); | ||
| } else { | ||
| setStatus(r, computeStatus(r)); | ||
| } |
There was a problem hiding this comment.
🧩 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:
- The shutdown check on line 83 counts only nodes with
FINISHEDstatus - If a node fails and remains in
RUNNINGstatus, the conditiongraph.getNodeCount() == graph.getNodeCountWithStatus(IDynamicGraph.Status.FINISHED)may never be satisfied - 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 20Length 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 20Length 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:
- The worker's tasks remain in
RUNNINGstatus (never transitioned toFINISHEDorREADY) - The shutdown check on line 83 requires all nodes to have
FINISHEDstatus - If any node fails with an exception and stays in
RUNNING, the condition is never satisfied - 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.
| 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.
|
Hi @krmahadevan can you look into this PR? What should I do to merge this? |
|
@lkalina-cera - Can you please run Also it would be good if we could please include a test that demonstrates that the fix works fine. |
2 participants
Fixes #3243 .
Did you remember to?
CHANGES.txt./gradlew autostyleApplyWe encourage pull requests that:
If your pull request involves fixing SonarQube issues then we would suggest that you please discuss this with the
TestNG-dev before you spend time working on it.
Note: For more information on contribution guidelines please make sure you refer our Contributing section for detailed set of steps.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.