Trigger fault sequence for forceful connection closures #2431
Conversation
WalkthroughAdded a public method Changes
Sequence Diagram(s)sequenceDiagram
participant Backend
participant TargetHandler
participant Connection
participant TargetErrorHandler
Backend->>TargetHandler: endOfInput(conn)
TargetHandler->>TargetHandler: Read ProtocolState & log diagnostics
TargetHandler->>Connection: Set state -> CLOSED
TargetHandler->>Connection: Shutdown (close) connection
alt previous state != RESPONSE_DONE && request msg ctx exists
TargetHandler->>TargetHandler: Mark internal origin = ERROR_HANDLER
TargetHandler->>TargetErrorHandler: forward(SND_IO_ERROR, current state)
else
TargetHandler->>TargetHandler: No error forwarding
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
modules/transports/core/nhttp/src/main/java/org/apache/synapse/transport/passthru/TargetHandler.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
modules/transports/core/nhttp/src/main/java/org/apache/synapse/transport/passthru/TargetHandler.java (2)
modules/transports/core/nhttp/src/main/java/org/apache/synapse/transport/passthru/TargetContext.java (1)
TargetContext(34-258)modules/transports/core/nhttp/src/main/java/org/apache/synapse/transport/passthru/PassThroughConstants.java (1)
PassThroughConstants(19-280)
🔇 Additional comments (1)
modules/transports/core/nhttp/src/main/java/org/apache/synapse/transport/passthru/TargetHandler.java (1)
1069-1086: Add metrics update and correlation logging toendOfInput()to match the error-handling pattern inexception().The review comment is valid. Verification confirms:
endOfInput()callsshutdownConnection(conn, true)which invokesconn.shutdown()rather thanconn.close(), preventing theclosed()callback from being triggered and bypassing themetrics.disconnected()call at line 832.
endOfInput()lacks correlation logging entirely, whereasexception()conditionally logs to correlation log when enabled (lines 1158-1160).Since
endOfInput()represents an error path (unexpected connection termination), it should mirror the telemetry pattern inexception(): addmetrics.disconnected()directly and conditionally log to correlation log usinglogHttpRequestErrorInCorrelationLog()whenPassThroughCorrelationConfigDataHolder.isEnable()is true.
...transports/core/nhttp/src/main/java/org/apache/synapse/transport/passthru/TargetHandler.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
modules/transports/core/nhttp/src/main/java/org/apache/synapse/transport/passthru/TargetHandler.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
modules/transports/core/nhttp/src/main/java/org/apache/synapse/transport/passthru/TargetHandler.java (2)
modules/transports/core/nhttp/src/main/java/org/apache/synapse/transport/passthru/TargetContext.java (1)
TargetContext(34-258)modules/transports/core/nhttp/src/main/java/org/apache/synapse/transport/passthru/PassThroughConstants.java (1)
PassThroughConstants(19-280)
| public void endOfInput(NHttpClientConnection conn) throws IOException { | ||
| conn.close(); | ||
|
|
||
| ProtocolState state = TargetContext.getState(conn); | ||
| log.warn("Connection ended unexpectedly" + | ||
| ", " + getConnectionLoggingInfo(conn) + | ||
| ", State: " + state + "."); | ||
| TargetContext.updateState(conn, ProtocolState.CLOSED); | ||
| targetConfiguration.getConnections().shutdownConnection(conn, true); | ||
| MessageContext requestMsgCtx = TargetContext.get(conn).getRequestMsgCtx(); | ||
| if (state != ProtocolState.RESPONSE_DONE && requestMsgCtx != null) { | ||
| requestMsgCtx.setProperty(PassThroughConstants.INTERNAL_EXCEPTION_ORIGIN, | ||
| PassThroughConstants.INTERNAL_ORIGIN_ERROR_HANDLER); | ||
| targetErrorHandler.handleError(requestMsgCtx, | ||
| ErrorCodes.SND_IO_ERROR, | ||
| "Error in Sender", | ||
| null, | ||
| state); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Verify pipe notification and metrics handling.
The implementation correctly addresses the PR objective by conditionally triggering the error handler only when state != ProtocolState.RESPONSE_DONE, preventing spurious error logs when the response is already complete.
However, comparing this with similar event handlers in the file (e.g., closed(), timeout(), exception()), I notice that those methods call:
informReaderError()orinformWriterError()based on state to notify pipes that an error occurred- Metrics methods like
metrics.disconnected()ormetrics.incrementFaultsReceiving()
The endOfInput method doesn't make these calls. While shutdownConnection(conn, true) and targetErrorHandler.handleError() might handle this internally, it's worth verifying that:
- Pipes are properly notified so that threads waiting on them don't hang
- Metrics are correctly updated for monitoring purposes
Run the following script to check how informReaderError and informWriterError are used in other event handlers and whether shutdownConnection handles pipe cleanup:
🏁 Script executed:
#!/bin/bash
# Check usage patterns of pipe notification and metrics in event handlers
echo "=== Event handlers that call informReaderError or informWriterError ==="
rg -n "def (closed|timeout|exception|endOfInput)" -A 50 | rg "(informReaderError|informWriterError|metrics\.)"
echo -e "\n=== Check if shutdownConnection handles pipe cleanup ==="
rg -n "shutdownConnection" -B 2 -A 2
echo -e "\n=== Check targetErrorHandler.handleError implementation ==="
ast-grep --pattern 'class TargetErrorHandler {
$$$
handleError($$$) {
$$$
}
$$$
}'Length of output: 37585
Let me search for the complete implementations in the codebase to verify the pattern.
Let me correct that and run with the repository:
🏁 Script executed:
#!/bin/bash
# Find the closed() and exception() method implementations in TargetHandler
echo "=== TargetHandler.closed() method implementation ==="
rg -n "public void closed" --type java -A 80 modules/transports/core/nhttp/src/main/java/org/apache/synapse/transport/passthru/TargetHandler.java | head -100
echo -e "\n=== TargetHandler.exception() method implementation (first 50 lines) ==="
rg -n "public void exception" --type java -A 50 modules/transports/core/nhttp/src/main/java/org/apache/synapse/transport/passthru/TargetHandler.java | head -60Length of output: 10675
🏁 Script executed:
#!/bin/bash
# Extract the closed() method from TargetHandler
echo "=== TargetHandler.closed() method ==="
sed -n '/^\s*public void closed(NHttpClientConnection/,/^\s*public void/p' modules/transports/core/nhttp/src/main/java/org/apache/synapse/transport/passthru/TargetHandler.java | head -100
echo -e "\n=== TargetHandler.exception() method (first 60 lines) ==="
sed -n '/^\s*public void exception(NHttpClientConnection/,/^\s*public void/p' modules/transports/core/nhttp/src/main/java/org/apache/synapse/transport/passthru/TargetHandler.java | head -100Length of output: 13628
Add pipe notifications and metrics update to prevent thread hangs and incomplete monitoring.
Verification shows that endOfInput() is missing critical operations found in similar event handlers:
-
Missing pipe notifications: Both
closed()(lines 781, 794) andexception()(lines 1095, 1106) callinformWriterError()orinformReaderError()for REQUEST_HEAD/REQUEST_BODY and RESPONSE_HEAD/RESPONSE_BODY states respectively. These notify the pipes and prevent threads from hanging. TheendOfInput()method omits these calls entirely. -
Missing metrics update: The
closed()method callsmetrics.disconnected()(line 832) for all closures. TheendOfInput()method has no equivalent metrics call.
The pattern from existing handlers is: Always notify pipes for relevant states, but conditionally call error handler based on state != RESPONSE_DONE. Currently, endOfInput() only does the conditional error handler part.
Fix required: Add pipe notifications and metrics update similar to closed() and exception() methods.
🤖 Prompt for AI Agents
In
modules/transports/core/nhttp/src/main/java/org/apache/synapse/transport/passthru/TargetHandler.java
around lines 1069 to 1087, endOfInput(...) currently logs, updates state, shuts
down the connection and conditionally invokes the error handler, but it omits
the pipe notifications and metrics update used by closed() and exception();
update endOfInput() to: call the same pipe notification helpers used elsewhere
(invoke informWriterError() for REQUEST_HEAD/REQUEST_BODY and
informReaderError() for RESPONSE_HEAD/RESPONSE_BODY as appropriate) before/after
updating state so waiting threads are unblocked, invoke metrics.disconnected()
to record the disconnect, and keep the existing conditional call to
targetErrorHandler only when state != RESPONSE_DONE; ensure ordering matches
closed()/exception() (notify pipes, update metrics, update state/shutdown
connection, then conditional error handler).
2 participants
Purpose
To fix wso2/api-manager#4414
Approach
Samples
N/A
Summary by CodeRabbit