[infinispan-issues] [JBoss JIRA] (ISPN-7889) BaseDistributionInterceptor.remoteGet may cause concurrency issues

[Dan Berindei (JIRA)]

    [ https://issues.jboss.org/browse/ISPN-7889?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13422354#comment-13422354 ] 

Dan Berindei commented on ISPN-7889:
------------------------------------

You're right, {{beforeGet}} is not thread-safe, and neither is its {{thenApply()}} callback.

I wonder if we need a method {{InvocationStage.afterBothAccept(InvocationStage, InvocationContext, VisitableCommand, InvocationSuccessAction)}} to emulate {{CompletableFuture.allOf}}, something like this:

{code}
InvocationStage remoteGet(..., InvocationStage previousRemoteGet) {
   ...
   return makeStage(asyncValue(rpcManager.invokeRemotely(...)).afterBothAccept(previousRemoteGet, ctx, command, this::storeRemoteValueInContext));
}
{code}

OTOH this is exactly the kind of thing I had in mind in ISPN-6971 when I made the calls to {{ResponseCollector}} methods protected by a lock, so if possible I'd use something like {{Transport.invokeCommands()}} (the corresponding {{RpcManager}} method doesn't exist yet) and save the entries in the context in the {{ResponseCollector}} implementation. 

Multiple invocations could also share the same collector, but then you have to somehow cancel the collector after the first exception. Or you could cache the exception in the collector, and only throw it after you got a reply from all the targets...

> BaseDistributionInterceptor.remoteGet may cause concurrency issues
> ------------------------------------------------------------------
>
>                 Key: ISPN-7889
>                 URL: https://issues.jboss.org/browse/ISPN-7889
>             Project: Infinispan
>          Issue Type: Bug
>          Components: Core
>    Affects Versions: 9.1.0.Alpha1
>            Reporter: Radim Vansa
>
> {{BaseDistributionInterceptor.remoteGet}} or any call that accesses the context in an async future handler that is called multiple times in parallel may lead to concurrent modifications of the context.
> These calls are usually handled using {{CompletableFuture.allOf()}} or using a CF with counter, but if one of the calls results in exceptional completion of the composed future, the processing continues (e.g. with a retry). The other parallel operation handlers are not stopped, though.
> {{BaseDistributionInterceptor.remoteGet}} shouldn't be called in parallel because it does not even synchronize regular successful invocations.
> A problem like this caused failures in {{GetAllCommandStressTest}}, and the issue was addressed for {{GetAllCommand}} in ISPN-7884.



--
This message was sent by Atlassian JIRA
(v7.2.3#72005)