[
https://issues.apache.org/jira/browse/CXF-9257?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Andriy Redko updated CXF-9257:
------------------------------
Affects Version/s: 3.6.13
4.1.9
> Client futures never complete if the async callback throws
> ----------------------------------------------------------
>
> Key: CXF-9257
> URL: https://issues.apache.org/jira/browse/CXF-9257
> Project: CXF
> Issue Type: Bug
> Components: JAX-RS, JAX-WS Runtime
> Affects Versions: 4.1.9, 4.2.4, 3.6.13
> Reporter: Markus Heiden
> Priority: Major
>
> If the callback passed to an asynchronous client invocation throws an
> unchecked exception, the future returned by the invocation is never
> completed. Threads blocked in {{Future.get()}} hang forever, and
> {{get(timeout)}} is the only way out.
> *Affected*
> * JAX-RS client: {{JaxrsClientCallback}} invokes
> {{InvocationCallback.completed()}} / {{failed()}} before completing its
> {{CompletableFuture}}. An exception thrown by the callback propagates out of
> {{handleResponse()}} / {{handleException()}}, so the future is never
> completed. For {{WebClient}} the call to {{handleResponse()}} in
> {{ClientAsyncResponseInterceptor}} is not even inside a try block, so nothing
> downstream can recover. The same applies to {{cancel()}} and to the
> interruption path of {{JaxrsResponseFuture.get()}}, which both invoke
> {{failed()}}.
> * JAX-WS client: {{JaxwsClientCallback}} invokes
> {{AsyncHandler.handleResponse()}} the same way. In addition, when the
> exception escapes, {{ClientImpl}} catches it and calls {{handleException()}},
> which invokes the same {{AsyncHandler}} a second time and may throw again.
> *Expected behaviour*
> The future completes in every case. If the callback throws while handling a
> successful response, the future completes exceptionally with that exception,
> which is what the JAX-WS reference implementation does
> ({{com.sun.xml.ws.client.AsyncResponseImpl.set}}). If the callback throws
> while handling a failure, the original failure is kept as the cause and the
> callback's exception is attached as a suppressed exception.
> *Context*
> Observed in the Microsoft Advertising Java SDK, which builds its own futures
> on top of the CXF async client: a runtime exception in its handler, e.g. from
> a missing response header, left the SDK's future unresolved forever. See
> https://github.com/BingAds/BingAds-Java-SDK/issues/166#issuecomment-2016613395
> and https://github.com/BingAds/BingAds-Java-SDK/issues/242.
> *Fix*
> https://github.com/apache/cxf/pull/3565 follows the existing CXF callback
> contract: callers invoke {{ClientCallback.handleException()}} if
> {{handleResponse()}} throws.
> * The call sites that don't follow the contract now do: {{WebClient}}'s
> {{ClientAsyncResponseInterceptor}} and the fault observer wrapper in
> {{ClientImpl}}.
> * The failure paths stay guarded inside {{JaxrsClientCallback}} and
> {{JaxwsClientCallback}} ({{handleException()}}, {{cancel()}}, the
> interruption path of {{JaxrsResponseFuture.get()}}). If the user's callback
> throws there, the original exception is kept and the callback's exception is
> attached as suppressed, unless it is the same instance (self-suppression
> would throw {{IllegalArgumentException}}).
> * Unit tests for both callbacks and a {{WebClient}} system test in
> {{JAXRSAsyncClientTest}} are included.
> *Approaches*
> The PR went through two revisions. The second one was chosen after review.
> _Approach 1 (first revision): guard every callback invocation inside the
> callbacks_
> {{JaxrsClientCallback}} and {{JaxwsClientCallback}} caught every exception
> thrown by the user's callback themselves. If the callback threw while
> handling a successful response, the future completed exceptionally with that
> exception. If it threw while handling a failure, the original exception was
> kept and the callback's exception was attached as suppressed.
> Upsides:
> * Self-contained: the future completes no matter how the callback is invoked,
> including future call sites that forget the try/catch.
> * The user's callback is invoked exactly once per invocation, like the JAX-WS
> reference implementation ({{com.sun.xml.ws.client.AsyncResponseImpl.set}}).
> Downsides:
> * Bypasses the existing contract (callers invoke {{handleException()}} if
> {{handleResponse()}} throws), so there are two mechanisms for the same thing
> and the callers' catch blocks become dead code for these callbacks.
> * Leaves the call-site gaps in {{WebClient}} and {{ClientImpl}} in place, so
> other {{ClientCallback}} implementations are still affected.
> _Approach 2 (current revision): follow the contract, fix the call sites,
> guard only the failure paths_
> Upsides:
> * Consistent with the existing design of {{ClientCallback}}; the success path
> in the callbacks is unchanged.
> * Fixes the call-site gaps for every {{ClientCallback}} implementation, not
> just the two callbacks.
> Downsides:
> * If the user's callback throws on a successful response, it is invoked a
> second time with the failure: the JAX-WS {{AsyncHandler}} first gets a
> successful and then a failed {{Response}}, the JAX-RS {{InvocationCallback}}
> gets {{completed()}} followed by {{failed()}}. The JAX-WS reference
> implementation invokes the handler only once.
> * The success path relies on every caller honoring the contract; a new call
> site without the try/catch reintroduces the hang.
> Why approach 2: the review on the PR preferred keeping the existing callback
> contract and fixing the call sites that violate it. The guards on the failure
> paths are needed with either approach: if {{handleException()}} throws, the
> caller has no way left to complete the future, because the future is owned by
> the callback. In the original Microsoft Advertising SDK case, the
> {{AsyncHandler}} throws again when it is invoked from {{handleException()}},
> so fixing the call sites alone would not have been enough.
> _Comparison_
> ||Aspect||Approach 1||Approach 2||
> |Future always completes|Yes|Yes, as long as callers honor the contract|
> |Exception of the future if the callback throws on success|The callback's
> exception|The callback's exception (via {{handleException()}})|
> |Invocations of the user's callback if it throws on success|Once|Twice
> (success, then failure)|
> |Same as the JAX-WS reference implementation|Yes|Same outcome of the future,
> but the handler is invoked twice|
> |Consistent with the existing {{ClientCallback}} contract|No|Yes|
> |Fixes the call-site gaps for other {{ClientCallback}} implementations|No|Yes|
> |Changes to the success path in the callbacks|Yes|No|
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
