[infinispan-dev] Sequential interceptors API

Wed Aug 3 07:54:41 EDT 2016

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. 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