[Issue 1487] Fix ErrorDecoder not invoked for Apache HttpClient checked exceptions

[gitshabh]

Problem

When Apache HttpClient throws checked exceptions like ProtocolException (e.g., missing Location header on 303 redirect), the ErrorDecoder was never invoked because the exception occurred during client.execute() before a Response object was created.

Root Cause

The ErrorDecoder interface requires a Response object: [Exception decode(String methodKey, Response response)]. When execution fails with checked exceptions, there's no Response to pass, causing the exception to propagate uncaught and potentially get wrapped in UndeclaredThrowableException.

Solution:

Build Response with appropriate status codes in case of ProtocolException instead of throwing exception so that error decoder can be invoked.

[gitshabh]

@velo
Hi, we can see the "Redirection but no location header" in the FeignException instead of UndeclaredThrowableException. Do you think this solution is correct?

[velo]

Thanks for picking this up, but I don't think the synthetic-response approach is the right direction. Concerns:

• Misleading status codes: 300 for "missing Location header" and 500 for arbitrary checked exceptions misrepresent what actually happened. ErrorDecoders keying off status will misclassify, and these synthetic codes can collide with real server responses the user already handles.
• Silent retry loss: callers previously got RetryableException (going through Retryer); now they get a non-retryable FeignException. That is a backwards-incompatible behavior change.
• Brittle detection: cause.getClass().getSimpleName().equals("ProtocolException") should be instanceof org.apache.http.ProtocolException. String comparison also collides with java.net.ProtocolException.
• Lossy: original cause is reduced to body bytes via getMessage().getBytes() (platform default charset). ErrorDecoders can't tell what actually went wrong.
• Catches too broadly: !IOException && !RuntimeException && !Error swallows any checked-exception cause into a fake 500, far wider than "ProtocolException on redirect".
• reason field misuse: that field is the HTTP reason phrase, not an exception class name.
• Cross-client inconsistency: only HC4 gets this treatment; hc5/OkHttp/JDK/Java11 still behave differently for the same scenario.
• Test coverage: one test, only the redirect path; the generic 500 branch is untested.

Posting better directions in a separate comment.

[velo]

@gitshabh thanks for the work — to your question, I don't think this is the right approach. Some better directions:

1. Minimal fix at the boundary. Ensure non-IOException causes get wrapped in IOException so the existing errorExecuting path in SynchronousMethodHandler (core/src/main/java/feign/SynchronousMethodHandler.java:115) handles them. This addresses the original UndeclaredThrowableException complaint without inventing fake responses or changing retry semantics:

try {
  HttpResponse httpResponse = client.execute(httpUriRequest);
  return toFeignResponse(httpResponse, request);
} catch (IOException e) {
  throw e;
} catch (Exception e) {
  throw new IOException(e);
}

That said — client.execute() already declares throws IOException, and Apache wraps ProtocolException in ClientProtocolException (which extends IOException). For the reported case the existing path should already fire and produce a RetryableException. Worth confirming the actual observed exception chain before patching, because the original UndeclaredThrowableException symptom may have a different root cause.

2. If you genuinely want ErrorDecoder to participate in transport-level failures, that's a Client-interface design discussion, not a per-client patch. The 2021 commits linked from #1487 explored extending ErrorDecoder to receive the original exception — that's a more honest model than fabricating an HTTP response. It should be applied consistently across all client modules (OkHttp/hc5/JDK/Java11), otherwise you create per-client behavior divergence.

3. Where the change belongs. If anything is centralized, it belongs in core (e.g., SynchronousMethodHandler broadening its catch and wrapping non-IOException causes), not duplicated in every Client implementation. Replicating the synthetic-response trick across hc5/OkHttp/JDK/Java11 would multiply the misleading-status-code and retry-loss problems across modules.

The fundamental issue with the current PR: synthetic HTTP statuses (300/500) are a lie about what happened, and they silently disable retries. Even just for HC4, those footguns outweigh the nicer error message.