issue 361 - async feign variant supporting CompleteableFutures #1174

motinis · 2020-02-17T16:39:01Z

Adds CompleteableFuture support.
The targeted API must be an interface, where each method has a return type of CompleteableFuture for some type T which can be decoded by the configured decoder.

Context is explicit, providing support for sessions.
A default synchronous -> asynchronous client is provided, although the different modules should add their own implementations of AsyncClient.

Retry is not supported - in particular, there can be many different ways one could handle the threading (scheduling the try on an appropriate thread). resilience4j could be an option, in particular to provide a wrapper to AsyncClient.

velo

I like this change... like to the point that I'm curious if it's possible to actually made it the main implementation. If we can get Async and non-async sharing a bit more of code, I think would be possible.

core/src/test/java/feign/AsyncFeignTest.java

velo · 2020-02-18T01:46:02Z

core/src/test/java/feign/AsyncFeignTest.java

+		CompletableFuture<Response> response();
+
+		@RequestLine("POST /")
+		CompletableFuture<String> post() throws TestInterfaceException;


I'm looking at this and I like it.

I wonder, do you think it would be possible to handle CompletableFuture<String> and String on the same code base?

So, deal with async and synchronous code using the same classes.

This would open the possibility of having async behavious as part of default feign, which would be interesting.

It is possible, and easy to implement. The proxy ReflectiveAsyncFeign would need to detect these kinds of methods and use CompleteableFuture.join() in a try/catch block with appropriate exception handling.

I though would exercise some caution here.

I think that interfaces should either be asynchronous or synchronous - I really can't think of a case where it makes sense to mix between the two.

If this is to be used as a replacement for the synchronous approach today, then I think it does require adding in a "blessed" way for doing retries (as exists today). Note though that it would break existing integrations, which would need to implement AsyncClient, as well as client code, in particular because context is now explicit. One could get around this by having the client also implement the context supplier (and handle under the covers in the builder by checking if the client implements Supplier<C>). This isn't too bad, since the synchronous clients today handle this internally generally on a thread local anyway.

I think that interfaces should either be asynchronous or synchronous - I really can't think of a case where it makes sense to mix between the two.

You may not, but our user's may, so we should consider it.

See my other comment on Retry. We will have to break it.

There is really only one reason that I can see to want a mixed interface and that's when wants to extend an existing synchronous interface to add on new async features (and one wants to be able to pass the instance to existing code).

Otherwise, it's generally trivial to do a join on the async call. The other way, changing client code from sync to async would likely take additional effort. But you're right that users could potentially have other constraints or reasons they need things a certain way.

However, it's not difficult to support - best practices can certainly be captured in documentation.

I think that interfaces should either be asynchronous or synchronous - I really can't think of a case where it makes sense to mix between the two.

My point at supporting both at same time, would be mainly to force a single workflow inside feign.

I don't really expect anyone using async for 3 methods and having one not async. Maybe someone will.

I added now sync through async. One thing that was necessary as part of that was coercing the SynchronousMethodHandler to explictly call the decoder (which is really the stageDecode in AsyncFeign) - this is because there were some edge cases in terms of Response and void return types which have correct special handling in AsyncResponseHandler which would be quite ugly to handle otherwise. I thought this way would be the most robust, absent a more extensive rewrite. To keep things consistent and simple, the coercion is also used for the async case, even though it works without it.

kdavisk6

@motinis

Awesome job here, really. We are very grateful for your work and spending the time to help us with this.

Before this can move forward, I think we have some foundational stuff to change. I agree with @velo that this should be our default way of processing requests. I've done some preliminary work on this in FeignX: https://github.com/OpenFeign/feignx/tree/master/core/src/main/java/feign/impl and I think we can combine the concepts here. Let's create a branch for us to collaborate on to allow us to work out these changes without blocking other bug fixes.

Here's what I think we can do

Refactor MethodMetadata to be thread-safe
Refactor handleResponse to be independent functions
Refactor Contract to choose the right InvocationHandler based on return type.

If we do these three things, this will lay the groundwork for everything else.

One last thought on Retry, this we know is something that will break when we do this and we know that.

core/src/main/java/feign/AsyncClient.java

kdavisk6 · 2020-02-18T14:40:25Z

core/src/main/java/feign/AsyncFeign.java

+
+	private static class LazyInitializedExecutorService {
+
+		private static final ExecutorService instance = Executors.newCachedThreadPool(r -> {


Cached Thread Pools are OK to start, but I think we may be better off with a Fixed Thread Pool.

I chose a CachedThreadPool to avoid needing to define additional configuration. Users can of course elect to pass in their own executor here for converting a sync client for use in async code (although they may have issues there with the fact that context may be shared in strange ways).

core/src/main/java/feign/AsyncFeign.java

core/src/main/java/feign/AsyncResponseHandler.java

kdavisk6 · 2020-02-18T14:45:58Z

core/src/main/java/feign/ReflectiveAsyncFeign.java

+
+public class ReflectiveAsyncFeign<C> extends AsyncFeign<C> {
+
+	private static class MethodInfo {


MethodMetadata already exists to serve this purpose. We should consider refactoring that component before creating additional components.

Agreed - it was doing much too much for my needs, hence the duplication. Much preferable to refactor.

motinis · 2020-02-18T15:35:26Z

Thank you for the positive feedback, and yes, I think a branch to work through this is a great idea. There are several aspects of Feign internals that I'm not familiar enough with, so please be patient if some questions look "too basic".

Here's what I think we can do

Refactor MethodMetadata to be thread-safe

Refactor handleResponse to be independent functions

Refactor Contract to choose the right InvocationHandler based on return type.

In terms of making MethodMetadata threadsafe - I have a feeling rather that this class should be split in two. One part should be completely immutable - it could potentially be created when target is called (alternatively it could be created at call time but cached). The second part could be created per request and would be mutable as part of the processing, but then I don't think it would need to be thread-safe. First part could still be called MethodMetadata or be renamed, second part I would think as RequestMethodMetadata
For handleResponse to be "independent" - could you please explain here. Do you mean having different method signatures for sync vs async?

velo · 2020-02-18T21:27:56Z

* n terms of making MethodMetadata threadsafe - I have a feeling rather that this class should be split in two. One part should be completely immutable - it could potentially be created when `target` is called (alternatively it could be created at call time but cached). The second part could be created per request  and would be mutable as part of the processing, but then I don't think it would need to be thread-safe. First part could still be called `MethodMetadata` or be renamed, second part I would think as `RequestMethodMetadata`

Agree. I poke around it recently and it has mutable and immutable data in a single class. Moving all mutable data to a new RequestMethodMetadata makes lot's of sense.

I guess one side effect of that, would be us cutting a feign 11 release =)

# Conflicts: # core/src/test/java/feign/AsyncFeignTest.java

motinis · 2020-02-19T11:11:02Z

I've now updated the commits to take into account some of the comments - in particular, AsyncResponseHandler is used internally by SynchronousMethodHandler for processing of the response.

Next up will be expanding Async to support synchronous methods (as I said, this should be pretty easy) - for testing, the tests in FeignTest not dealing with Retry can be used, just going through AsyncFeign instead.

Finally would be looking to take MethodInfo and pull it out into first class citizen and then having MethodMetadata reference it, or extend it as the case may be.

I'm trying to get this pretty clean for all the non-retry before looking to do major rewriting.

velo · 2020-02-19T19:10:57Z

core/src/main/java/feign/AsyncFeign.java

+
+    private final Builder builder;
+    private Supplier<C> defaultContextSupplier = () -> null;
+    private AsyncClient<C> client;


I was also thinking in collapsing this functionality into feign core.

Feign would only include the AsyncClient attribute with two setters:

client(AsyncClient<C> client): just use AsyncClient leveraging the async behaviour for async methods or .join() for sync methods

client(Client client): regular client it gets warped around the AsyncClient you made, running on local thread

Sure - that could work - although there would need to be enforcement that then you're not using a context supplier (i.e., the client would always be called with an empty requestContext Optional)

I delivered AysncClient.Pseudo instead as the fully synchronous AsyncClient implementation.

Fair point, that is an excellent delivery. We can expand on that later

velo · 2020-02-19T23:36:13Z

BTW, is there any tasks you would like coding assistance with?

Feel free to throw anything you need at me, I can make PRs to your branch to help out

motinis · 2020-02-20T07:22:49Z

BTW, is there any tasks you would like coding assistance with?

Feel free to throw anything you need at me, I can make PRs to your branch to help out

Originally, I had this added as a "clean" extension - it leveraged standard Feign internally, but didn't make any changes. Since we're unifying the sync path onto this, it does require changes, and so we should do some cleanup here which I'd appreciate assistance with. One thing is I wouldn't want to break any existing code using the Feign class right now. What I suggest is to do this in 2 steps:

Instead of the forceDecoding option, have the ability to provide a MethodHandlerFactory as an internal part of Feign.Builder. This way, the Async codepath can be cleaned up from the stagedExecute and stagedDecode (these should be combined into a single method that's basically the AsyncMethodHandler implementation).
Refactor out a FeignDispatcher part - this should cover from proxy through to MethodMetadata creation at minimum, and probably including some more generic hook to the dispatch via a MethodHandler. AsyncFeign would then use that internally instead of Feign. Feign too should leverage FeignDispatcher (although whether via inheritance or other means is unclear). FeignDispatcher could possibly be public API, resulting in a separation of dispatch from handling.

velo · 2020-02-21T21:57:25Z

Originally, I had this added as a "clean" extension

You are right.
Let's get this in, as is (or at least w/o Wishlist) and cut a release. Put it out there in front of people =)
@kdavisk6 what do you think?

I made a few minor changes to your code that should make travis CI happy
motinis#1

The only think missing is some instructions on readme =)

Async

motinis · 2020-02-23T11:14:45Z

I made a few minor changes to your code that should make travis CI happy
motinis#1

Thanks, I've merged those in.

velo · 2020-02-23T14:47:43Z

There are still some formatting problems on core/src/main/java/feign/ReflectiveAsyncFeign.java running maven locally should fix it.

motinis · 2020-02-24T09:28:56Z

There are still some formatting problems on core/src/main/java/feign/ReflectiveAsyncFeign.java running maven locally should fix it.

Can you please do that and I'll merge? Getting an error on my local where it can't find pomSortOrder.xml in the classpath.

velo · 2020-02-24T09:34:32Z

Could you please run maven with -e and post the exception?

motinis · 2020-02-24T10:43:15Z

Could you please run maven with -e and post the exception?

I seem to be able to work-around this (by running from the feign parent dir with -f to core/pom.xml) - but the change it made to this file seems odd, in that it moved the opening brace to its own indented line:

      if (!CompletableFuture.class.isAssignableFrom(retType))
       {
        continue; // synchronous case
      }

If that's what you want, then sure I can push that...

velo · 2020-02-24T18:12:36Z

Odd, but let's see if that fixes the build

motinis · 2020-02-24T19:54:51Z

Looks like it did fix the build

velo · 2020-02-24T20:09:34Z

@kdavisk6 I'm happy to merge this as is and start working on feign 11 with

Refactor MethodMetadata to be thread-safe
Refactor handleResponse to be independent functions
Refactor Contract to choose the right InvocationHandler based on return type.

What do you think?

kdavisk6 · 2020-02-24T21:26:34Z

I’ll take a look tonight, but I trust you. Let’s create a new 11.x branch after this is merged and refactor there.

…

On Mon, Feb 24, 2020 at 3:09 PM Marvin Froeder ***@***.***> wrote: @kdavisk6 <https://github.com/kdavisk6> I'm happy to merge this as is and start working on feign 11 with - Refactor MethodMetadata to be thread-safe - Refactor handleResponse to be independent functions - Refactor Contract to choose the right InvocationHandler based on return type. What do you think? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1174?email_source=notifications&email_token=ABN4FOZEGAYS7DEI2EFBMA3REQSP5A5CNFSM4KWVBDMKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEMZLHGQ#issuecomment-590525338>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABN4FO6HMRNNOUUFBHDNO4TREQSP5ANCNFSM4KWVBDMA> .

velo · 2020-02-24T21:36:42Z

Ok, once merged, I think we should release another 10.x

motinis · 2020-02-26T11:03:54Z

@kdavisk6 I'm happy to merge this as is and start working on feign 11 with

Refactor MethodMetadata to be thread-safe

Refactor handleResponse to be independent functions

Refactor Contract to choose the right InvocationHandler based on return type.

What do you think?

I think the actual refactoring should look different... basically, the current method handler approach should be used. This depends then if you want to remove the current SynchronousMethodHandler or not.
Basically, there should be an AsyncMethodHandler and corresponding factory that gets used. The client, context supplier, etc. would get pushed down into the factory to pass over to the handler. There may need to be some work on fixing up MethodMetadata, but otherwise the determining of the method call mode (sync vs async) and underlying return type for async could be done in the method handler.
If one wants to keep around the existing SynchronousMethodHandler, I would basically suggest having the builder take a supplier for the factory (so like a factory factory), so the creation can be delayed until build is actually taking place. Then the default could be to create the existing Factory, while it could be overridden by async builder to create the async factory version.
Probably needs some diagrams, etc. and should be started in a new issue and branch :)

kdavisk6 · 2020-02-27T00:52:33Z

@motinis and @velo I've approved this PR. Feel free to move forward with this. I'll create a new issue for us to discuss how to move forward with Feign 11

velo · 2020-02-27T01:38:58Z

cool, I merged as is. Will work on releasing later today (social life permitting =] )

I will be sending a PR that added async capabilities to apache http 5 integration.

@motinis lemme know if you would be interested in reviewing that one.

nallabheem · 2021-05-06T07:54:02Z

Hi @velo, @motinis , is Async is available for Feign-Retry now. We are currently using OpenFeign3.0.0 and If we want to use the version of OpenFeign which support Async for Feign-Retry which version is that ? and what are all the changes between version 3.00 and 11 .

Could you please help me with the info.
Thanks in advance for the help and support.

velo · 2021-05-06T21:43:07Z

Hi @nallabheem , feign 3 is very, very, very, veeery old.

I can't start listing what would be the impact of such change. Most relevant thing that come to mind is that we dropped java 6 support. I would suggest you go ahead and try it.

@experimental

* issue 361 - async feign variant supporting CompleteableFutures * Update core/src/test/java/feign/AsyncFeignTest.java * remove duplication between synchronous and async cases * remove duplication between synchronous and async cases * support sync calls in async-feign * Add a synchronous "AsyncClient" * Added license header * Apply feign formatter * Add '@experimental' annotation to new async classes * "fix" formatting to try and appease build Co-authored-by: Marvin Froeder <velo@users.noreply.github.com>

@experimental

* issue 361 - async feign variant supporting CompleteableFutures * Update core/src/test/java/feign/AsyncFeignTest.java * remove duplication between synchronous and async cases * remove duplication between synchronous and async cases * support sync calls in async-feign * Add a synchronous "AsyncClient" * Added license header * Apply feign formatter * Add '@experimental' annotation to new async classes * "fix" formatting to try and appease build Co-authored-by: Marvin Froeder <velo@users.noreply.github.com>

issue 361 - async feign variant supporting CompleteableFutures

d52f15b

motinis mentioned this pull request Feb 17, 2020

Non-blocking I/O support #361

Open

velo reviewed Feb 18, 2020

View reviewed changes

Update core/src/test/java/feign/AsyncFeignTest.java

d94e928

kdavisk6 requested changes Feb 18, 2020

View reviewed changes

Moti Nisenson-Ken added 3 commits February 19, 2020 13:02

remove duplication between synchronous and async cases

d3f1373

Merge remote-tracking branch 'origin/master'

477569e

# Conflicts: # core/src/test/java/feign/AsyncFeignTest.java

remove duplication between synchronous and async cases

24eedcc

support sync calls in async-feign

b800a98

velo reviewed Feb 19, 2020

View reviewed changes

Moti Nisenson-Ken and others added 4 commits February 20, 2020 20:39

Add a synchronous "AsyncClient"

607ee73

Added license header

7a05185

Apply feign formatter

14d3c95

Add '@experimental' annotation to new async classes

63877dc

Merge pull request #1 from velo/async

e046ddb

Async

"fix" formatting to try and appease build

367bb37

velo added documentation Issues that require updates to our documentation enhancement For recommending new capabilities ready to merge Will be merged if no other member ask for changes waiting for votes Enhancements or changes proposed that need more support before consideration labels Feb 24, 2020

kdavisk6 approved these changes Feb 27, 2020

View reviewed changes

velo merged commit b6db37e into OpenFeign:master Feb 27, 2020

dependabot bot mentioned this pull request Mar 15, 2021

build(deps): bump feign.version from 10.7.0 to 11.1 Activiti/activiti-cloud-application#308

Closed


		private static class LazyInitializedExecutorService {

		private static final ExecutorService instance = Executors.newCachedThreadPool(r -> {


		public class ReflectiveAsyncFeign<C> extends AsyncFeign<C> {

		private static class MethodInfo {

issue 361 - async feign variant supporting CompleteableFutures #1174

issue 361 - async feign variant supporting CompleteableFutures #1174

Conversation

motinis commented Feb 17, 2020

velo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

motinis Feb 18, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kdavisk6 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

motinis commented Feb 18, 2020

velo commented Feb 18, 2020

motinis commented Feb 19, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

velo commented Feb 19, 2020

motinis commented Feb 20, 2020

velo commented Feb 21, 2020 • edited Loading

motinis commented Feb 23, 2020

velo commented Feb 23, 2020

motinis commented Feb 24, 2020

velo commented Feb 24, 2020

motinis commented Feb 24, 2020

velo commented Feb 24, 2020

motinis commented Feb 24, 2020

velo commented Feb 24, 2020

kdavisk6 commented Feb 24, 2020 via email

velo commented Feb 24, 2020

motinis commented Feb 26, 2020

kdavisk6 commented Feb 27, 2020

velo commented Feb 27, 2020

nallabheem commented May 6, 2021

velo commented May 6, 2021

motinis Feb 18, 2020 •

edited

Loading

velo commented Feb 21, 2020 •

edited

Loading