[infinispan-dev] Sequential interceptors API
Galder Zamarreño
galder at redhat.com
Thu Aug 4 03:03:10 EDT 2016
--
Galder Zamarreño
Infinispan, Red Hat
> On 3 Aug 2016, at 13:54, Dan Berindei <dan.berindei at gmail.com> wrote:
>
> Yeah, I just fixed (almost all of) the test failures this morning and
> I pushed it here:
>
> https://github.com/infinispan/infinispan/compare/master...danberindei:ISPN-5467_CompletableFuture-like_API?expand=1
>
>
> Galder, I'm not sure I understand the part about mixing promises and
> callbacks... almost all CompletableFuture methods take a callback
> parameter, so I'm not sure how one could avoid mixing them.
The question how these callbacks are passed:
When using Promises or CompletableFutures, the callbacks are passed to the promise/CF via its fluent API, e.g:
CompletableFuture cf = methodA();
cf.thenCompose().thenCombine().handle().whenComplete()... // Might not be realistic, just showing fluency in action
In your case, the forkHandler is passed in as parameter to methodA() instead of it being a continuation that you add to the CompletableFuture via its fluent API.
Passing handlers as parameters to a method you created (and not as parameters to a continuation of CompletableFuture) is the old asynchronous call handling method that Node.js made so famous. The current API mixes both, by passing some callbacks to CompletableFuture methods and then some being passed around as method parameters, which makes it ackward.
So, instead of:
CompletableFuture<Void> forkInvocation(VisitableCommand newCommand, AsyncInterceptor.ForkReturnHandler forkReturnHandler)
I'd expect:
CompletableFuture<Z> forkInvocation(VisitableCommand newCommand);
And use it like this:
CompletableFuture<Z> cf = forkInvocation(cmd);
cf.whenComplete(forkReturnHandler); //
So, what is forkReturnHandler? forkReturnHandler would be a BiConsumer<Z, Throwable>.
Now, what is Z? Z is everything that the handler needs to deal with, except Throwable, which gets passed as 2nd element to the BiConsumer. So Z would a wrapper around InvocationContext, VisitableCommand and Object (return value).
> That being
> said, the API as it is in master now tends to encourage nesting of
> callbacks, and that's what I'm trying to change.
>
> My new approach has a promise-like interface called
> IntermediateInvocationStage (for now), which works like a promise on
> the interceptors not yet executed. It has various methods you can then
> use to add callbacks to be executed after those interceptors (and any
> callbacks they installed).
>
> To get an IntermediateInvocationStage, you need to call a method like
> invokeNext(ctx, command) on BaseAsyncInterceptor/InterceptorStage. I'm
> not yet sure whether exposing InterceptorStage to the interceptor is
> really necessary.
>
> Both the interceptor and the callbacks then return an InvocationStage,
> which is implemented by the same classes that implement
> IntermediateInvocationStage, but doesn't allow adding more callbacks.
> This is mainly so that I can have returnWith(returnValue) instead of
> returnWith(ctx, command, returnValue).
>
> For the actual asynchronous work, I have 2 goAsync methods in
> InterceptorStage, because I think having an explicit callback can be
> slightly more efficient - I'm not 100% sure about this either. I used
> to have a waitFor(CompletionStage<?>) method that returned an
> InterceptorStage, but I decided it was too confusing to have
> InterceptorStage behave like a promise as well.
>
> IntermediateInvocationStage also allows "asynchronous" callbacks with
> composeAsync, but it seems a bit weird to have a single callback
> handle both on an InvocationStage and on a CompletionStage. So I'm
> thinking of removing that, and forcing the interceptors to use
> compose+goAsync instead. That probably means callback nesting again,
> so I need to check how it would affect the L1 interceptors.
>
> About performance: I only tested non-tx local reads so far, but in
> that case I'm only getting one real allocation in CallInterceptor,
> which gets reused by all the other interceptors. For this scenario all
> the callbacks are cached in the interceptor fields, of course, so it's
> not HotSpot doing scalar replacement for the callbacks - although in
> theory that should work as well. I need to do a lot more testing on
> this...
>
>
> Cheers
> Dan
>
>
>
> On Wed, Aug 3, 2016 at 1:49 PM, Radim Vansa <rvansa at redhat.com> wrote:
>> I think that Dan had another branch with more FSM-like API, but I can't
>> find it :-/
>>
>> R.
>>
>> On 08/03/2016 11:57 AM, Galder Zamarreño wrote:
>>> Hey Radim,
>>>
>>> Sorry for the delay having a look at this (Summit/DevNation then vacation).
>>>
>>> I agree with you that the way you're passing down the handler down the stack and accross lambdas is a bit counter-productive, because it means the lambda has to capture the handler, which in turn means that each lambda execution will result in a lambda instantiation.
>>>
>>> Looking at AsyncInvocationContext, methods like this feel a bit of an anti-pattern:
>>>
>>> CompletableFuture<Void> forkInvocation(VisitableCommand newCommand, AsyncInterceptor.ForkReturnHandler forkReturnHandler)
>>>
>>> The problem here is that it mixes the two main ways of dealing with async computation:
>>>
>>> 1. Callback handlers [1].
>>> 2. Promises (~ CompletableFuture) [2].
>>>
>>> Normally, you either use one or the other, but here both are in use because the method returns a CompletableFuture (e.g. like a JS/Scala promise), and then is taking a callback handler as well.
>>>
>>> If you go down the path of promises, the normal thing would be:
>>>
>>> CompletableFuture<Void> forkInvocation(VisitableCommand newCommand)
>>>
>>> And then you chain (e.g. thenCompose or similar) the return function when the fork has completed. I assume that going down this route causes unnecessary allocations?
>>>
>>> If we're worried about allocations, I wonder whether a pure handler based approach might work better? It might not be as user-friendly but if we want to avoid CompletableFuture issues internally, then maybe that's the way? e.g.
>>>
>>> void forkInvocation(VisitableCommand newCommand, AsyncInterceptor.ForkReturnHandler forkReturnHandler);
>>>
>>> While at Summit/DevNation I had a chat with Clement from Vert.x team who had some reservations about CompletableFutures themselves. I don't remember the details right now (that's what vacation does to you!) but I've pinged him to find out more.
>>>
>>> In general though, there are multiple issues associated with the handler approach, including:
>>>
>>> * Lack of central error handling logic.
>>> * Callback readibility, when multiple handlers are specified in one go [3].
>>>
>>> Cheers,
>>>
>>> [1] http://www.servicedesignpatterns.com/WebServiceInfrastructures/AsyncResponseHandler
>>> [2] http://exploringjs.com/es6/ch_promises.html
>>> [3] http://callbackhell.com
>>> --
>>> Galder Zamarreño
>>> Infinispan, Red Hat
>>>
>>>> On 22 Jun 2016, at 17:52, Radim Vansa <rvansa at redhat.com> wrote:
>>>>
>>>> Yes [1]. The longest chaining of operations I had in [2], basically
>>>> during ST I have to load a value locally*, perform a unicast/broadcast
>>>> to read different value and then execute the original one.
>>>>
>>>> * I shouldn't load it just from DC, as it could be in cache store, too;
>>>> though without persistence (which I don't deal with properly in
>>>> scattered cache yet) it would be more efficient to do the DC lookup
>>>> directly.
>>>>
>>>> Radim
>>>>
>>>> [1] https://github.com/rvansa/infinispan/tree/ISPN-6645
>>>> [2]
>>>> https://github.com/rvansa/infinispan/blob/ISPN-6645/core/src/main/java/org/infinispan/interceptors/impl/PrefetchInvalidationInterceptor.java
>>>>
>>>> On 06/17/2016 10:52 AM, Galder Zamarreño wrote:
>>>>> Radim, do you have a branch where you have been trying these things out? I'd like to play with what you're trying to do.
>>>>>
>>>>> Cheers,
>>>>> --
>>>>> Galder Zamarreño
>>>>> Infinispan, Red Hat
>>>>>
>>>>>> On 8 Jun 2016, at 14:23, Radim Vansa <rvansa at redhat.com> wrote:
>>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> I would like to encourage you to play with the (relatively) new API for
>>>>>> sequential interceptors, and voice your comments - especially you corish
>>>>>> devs, and Galder who has much experience with async invocations and that
>>>>>> kind of stuff from JS-world.
>>>>>>
>>>>>> I am now trying to use the asynchronous methods only (the
>>>>>> forkInvocationSync() is only temporary helper!); Dan has made it this
>>>>>> way as he wanted to avoid unnecessary allocations, and I welcome this
>>>>>> GC-awareness, but regrettably I find it rather hard to use, due to its
>>>>>> handler-style nature. For the simplest style interceptors (do this,
>>>>>> invoke next interceptor, and process result) it's fine, but when you
>>>>>> want to do something like:
>>>>>>
>>>>>> visitFoo(cmd) {
>>>>>> Object x = null;
>>>>>> if (/* ... */) {
>>>>>> x = invoke(new OtherCommand());
>>>>>> }
>>>>>> invoke(new DifferentCommand(x));
>>>>>> Object retval = invoke(cmd);
>>>>>> return wrap(retval);
>>>>>> }
>>>>>>
>>>>>> I find myself passing handlers deep down. There is allocation cost for
>>>>>> closures, so API that does not allocate CompletableFutures does not pay off.
>>>>>>
>>>>>> I don't say that I could improve it (I have directed my comments to Dan
>>>>>> on IRC when I had something in particular), I just say that this is very
>>>>>> important API for further Infinispan development and everyone should pay
>>>>>> attention before it gets final.
>>>>>>
>>>>>> So please, play with it, and show your opinion.
>>>>>>
>>>>>> Radim
>>>>>>
>>>>>> --
>>>>>> Radim Vansa <rvansa at redhat.com>
>>>>>> JBoss Performance Team
>>>>>>
>>>>>> _______________________________________________
>>>>>> infinispan-dev mailing list
>>>>>> infinispan-dev at lists.jboss.org
>>>>>> https://lists.jboss.org/mailman/listinfo/infinispan-dev
>>>>> _______________________________________________
>>>>> infinispan-dev mailing list
>>>>> infinispan-dev at lists.jboss.org
>>>>> https://lists.jboss.org/mailman/listinfo/infinispan-dev
>>>>
>>>> --
>>>> Radim Vansa <rvansa at redhat.com>
>>>> JBoss Performance Team
>>>>
>>>> _______________________________________________
>>>> infinispan-dev mailing list
>>>> infinispan-dev at lists.jboss.org
>>>> https://lists.jboss.org/mailman/listinfo/infinispan-dev
>>>
>>> _______________________________________________
>>> infinispan-dev mailing list
>>> infinispan-dev at lists.jboss.org
>>> https://lists.jboss.org/mailman/listinfo/infinispan-dev
>>
>>
>> --
>> Radim Vansa <rvansa at redhat.com>
>> JBoss Performance Team
>>
>> _______________________________________________
>> infinispan-dev mailing list
>> infinispan-dev at lists.jboss.org
>> https://lists.jboss.org/mailman/listinfo/infinispan-dev
>
> _______________________________________________
> infinispan-dev mailing list
> infinispan-dev at lists.jboss.org
> https://lists.jboss.org/mailman/listinfo/infinispan-dev
More information about the infinispan-dev
mailing list