Skip to content

Trigger fault sequence for forceful connection closures #2431

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
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Trigger fault sequence for forceful connection closures #2431

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
30fa77d
Trigger fault sequence for forceful connection closures
PasanT9 Jun 2, 2025
ab28b09
Skip fault sequence for completed response states
Piumal1999 Nov 17, 2025
f3739f9
Fix inconsistency in error log
Piumal1999 Nov 17, 2025
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
26 changes: 25 additions & 1 deletion ...nsports/core/nhttp/src/main/java/org/apache/synapse/transport/passthru/TargetHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1058,8 +1058,32 @@ private void informWriterError(NHttpClientConnection conn) {
}
}

/**
* Invoked when the backend terminates the outbound HTTP connection unexpectedly.
* Logs diagnostics, marks the connection CLOSED, shuts it down, and routes the error
* to the standard fault handler.
*
* @param conn the target {@link NHttpClientConnection} that ended input
* @throws IOException if an error occurs while closing the connection
*/
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);
}
}
Comment on lines 1069 to 1087
Copy link

@coderabbitai coderabbitai bot Nov 17, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

🧩 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:

  1. informReaderError() or informWriterError() based on state to notify pipes that an error occurred
  2. Metrics methods like metrics.disconnected() or metrics.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 -60

Length 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 -100

Length 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:

  1. Missing pipe notifications: Both closed() (lines 781, 794) and exception() (lines 1095, 1106) call informWriterError() or informReaderError() for REQUEST_HEAD/REQUEST_BODY and RESPONSE_HEAD/RESPONSE_BODY states respectively. These notify the pipes and prevent threads from hanging. The endOfInput() method omits these calls entirely.

  2. Missing metrics update: The closed() method calls metrics.disconnected() (line 832) for all closures. The endOfInput() 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).


public void exception(NHttpClientConnection conn, Exception ex) {
Expand Down
Loading