[
https://issues.apache.org/jira/browse/CXF-9257?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Markus Heiden updated CXF-9257:
-------------------------------
Description:
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|
was:
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 guards every callback invocation in
both callbacks and adds unit tests, which fail without the fix.
> 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.2.4
> 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)
