ListenableFutureAdapter.addCallback may or may not propagate failures from a FailureCallback in the calling thread [SPR-13785]

[spring-projects-issues]

Scott Feldstein opened SPR-13785 and commented

There is a problem in AsyncRestTemplate where it does not consistently propagate an exception up the stack from the error callback.

I am attaching code to reproduce the issue.

Import the spring starter project into STS and then run the AsyncrestremplatebugApplicationTests.

The test method bug() will fail while the test method works() will pass. The only difference btwn the methods is that there is a sleep in the works() method. The sleep allows the rest call time to fail so the subsequent call to future.get() will immediately call the error callback.

In the bug() method the rest call does not complete before the error callback is added and therefore it fails. I used Thread.sleep() in my Controller to ensure that the callbacks are added before the rest call is completed.

I would expect the same the exception from the failure callback would be consistently thrown.

---

Affects: 4.1.6, 4.2.3

Attachments:

• asyncrestremplatebug.tgz (53.44 kB)

Issue Links:

• AsyncResult calls SuccessCallback when it should call FailureCallback [SPR-14249] #18822 AsyncResult calls SuccessCallback when it should call FailureCallback

[spring-projects-issues]

Rossen Stoyanchev commented

I've created a project in the spring-framework-issues repository based from the attached project. First a couple of notes. The 1 second delay in EndpointController makes no difference because the failure "401 Unauthorized" is from a Spring Security filter and the controller is never reached. The 5 second delay in works() seems to have been misplaced. The test didn't succeed for me until I moved it before the failure callback is added as shown in this commit.

The bottom line is, any failures from a FailureCallback will be raised in the thread that calls addCallback when the underlying result is ready. Or if the underlying result is not ready the FailureCallback will be added at first but will later fail in an executor thread when the underlying result becomes available.

Currently we make no effort to catch or handle failures from a FailureCallback itself. So it's not surprising you may or may not get them in the calling thread. The Javadoc makes no claim either way but it does not allow for checked Exception's to be thrown and the implicit expectation is that a FailureCallback will not fail. Can you elaborate if the exception from the FailureCallback is intentional in which case what is the intent of what you're trying to do? It would make sense for the FailureCallback to not fail itself.

If we were to change and deal with failures from a FailureCallback and make addCallback behave consistently, since that may happen before the underlying result is ready, the only choice we'd have is to make it never raise an exception.

[spring-projects-issues]

Rossen Stoyanchev commented

Updated title to reflect more accurately the root cause which is in ListenableFutureAdapter (and base classes) and is not caused by anything specific in the way the AsyncRestTemplate uses it.

[spring-projects-issues]

Juergen Hoeller commented

Alright, let's schedule this for a revision in 4.3, aiming for consistent treatment of such unexpected exceptions...

Juergen

[spring-projects-issues]

Scott Feldstein commented

Thanks Rossen, sorry for the confusion on the timeouts. It was kind of painful to get this to reproduce consistently so my implementation may have diverged with what was in my head :-)

Additionally, I just looked at your commit and the ExecutionException that you noted is thrown is behavior that I noticed diverges when I upgraded to spring-boot 1.3.0. Before that the Exception that is thrown is HttpClientErrorException (FYI)

The Exceptions that are thrown are internal to my framework which aligns it to behave the same as our synchronous apis. They are basically RuntimeExceptions that tell our mirco-service client libs to retry when the sso tokens have been invalidated (due to timeout or other).

I see what you are saying about why we wouldn't have been able to capture the exception in the calling thread due to the asynchronous nature of the request. If your ultimate goal of this is to make it consistent so that the calling thread never raises the exception - can you let me know what your thoughts are to be able to provide a generic retry mechanism like I described above for the async libs? What would be the problem with throwing the exception in the calling thread in addition to a thread pool?

thanks for the quick response on this!

[spring-projects-issues]

Rossen Stoyanchev commented

The idea with ListenerFuture is that you can get an async (success or failure) notification and that may be in another thread. You can use that callback to set some shared variable such as a CountdownLatch or you can use the blocking ListenerFuture.get() which will raise an ExecutionException with the actual failure inside it. Either way raising an Exception from an async callback is not a good idea and can not be made to work.

[spring-projects-issues]

Scott Feldstein commented

Thanks for the response. I know there are many ways to solve this but I wanted something a little more elegant than a latch that works directly with the framework. The problem is that it doesn't quite fit into the spring-retry mechanism, nor does retry accept spel so I can't use it to figure out if the HttpClientErrorException is an auth exception to retry.

I'll just write an one off aspect to implement the retry.

I'm just curious why you say that it is not a good idea and can not be made to work? I don't see why adding any callback exceptions that were caught in the async call into the some extension of ExecutionException wouldn't work. It would be nice for a consumer of the api to be aware of any exceptions that were thrown in the callbacks.

[spring-projects-issues]

Rossen Stoyanchev commented

I can't give better advice from a general description. The way java.util.Future works is the way it works. You get a value or an exception from some operation. That's set atomically and it's immutable. I'm not sure what exactly you're proposing? ListentableFuture only exposes a notification on top of that tells you if there was success or failure. Why do you need to raise an exception at that point? You're being notified of an exception.

[spring-projects-issues]

Scott Feldstein commented

I understand that is the case with the Future but in general it is reasonable to allow a caller to be aware of any exceptions that occur in the callbacks without having to write any extra infrastructure.

In our case the microservice architecture uses various auth tokens for inter-service communication. There are scenarios where we receive an auth failure. In that case we need to regenerate the token and retry the request. The HttpClientErrorException contains this information via the getHttpStatus() method. When this occurs we need to simply regenerate the token and retry. In the synchronous calls it is very easy to accomplish this by wrapping the HttpClientErrorException in some known RetryableException which is recognized by spring-retry but using async calls it is trickier.

If spring-retry allowed us to inspect the exception via some SPEL expression that would work as well. I can write something to accomplish this but my hope was that the framework would be able to handle it.

[spring-projects-issues]

Rossen Stoyanchev commented

Well a ListenableFuture is a Future. That's my point. So you'll have to be more specific what exactly you're proposing.

[spring-projects-issues]

Scott Feldstein commented

Gotcha. So you are saying that your hands are tied by the spec. That's fine - I'll request an enhancement in spring-retry.

thanks.

[spring-projects-issues]

Rossen Stoyanchev commented

See commit 3dae3f.