Re: [Tarantool-patches] [PATCH v2 2/4] replication: fix appliers pruning

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Thanks for the patch!

On 23/11/2019 22:53, Ilya Kosarev wrote:
> During pruning of replicaset appliers some anon replicas might connect
> from replicaset_follow called in another fiber. Therefore we need to
> prune appliers of anon replicas first.
> 
> Closes #4643
> ---
>  src/box/replication.cc | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/src/box/replication.cc b/src/box/replication.cc
> index 509187cd3..3ccfa8b33 100644
> --- a/src/box/replication.cc
> +++ b/src/box/replication.cc
> @@ -516,23 +516,23 @@ replicaset_update(struct applier **appliers, int count)
>  	 */
>  
>  	/* Prune old appliers */
> -	replicaset_foreach(replica) {
> -		if (replica->applier == NULL)
> -			continue;
> +	rlist_foreach_entry_safe(replica, &replicaset.anon, in_anon, next) {

'safe' rlist iteration won't help, if a next anon replica
will connect during yield in applier_stop(). Safe iteration can
cope with deletion only of the current element.

You can try

    while (rlist_empty(...)) {
        replica = rlist_first(...);
        ...
    }

The same about the cycle below.

And I am not really sure, that you need two
cycles now. Just keep it one, but iterate like
I showed above.

>  		applier = replica->applier;
>  		replica_clear_applier(replica);
> -		replica->applier_sync_state = APPLIER_DISCONNECTED;
> +		replica_delete(replica);
>  		applier_stop(applier);
>  		applier_delete(applier);
>  	}
> -	rlist_foreach_entry_safe(replica, &replicaset.anon, in_anon, next) {
> +	rlist_create(&replicaset.anon);
> +	replicaset_foreach(replica) {
> +		if (replica->applier == NULL)
> +			continue;
>  		applier = replica->applier;
>  		replica_clear_applier(replica);
> -		replica_delete(replica);
> +		replica->applier_sync_state = APPLIER_DISCONNECTED;
>  		applier_stop(applier);
>  		applier_delete(applier);
>  	}
> -	rlist_create(&replicaset.anon);
>  
>  	/* Save new appliers */
>  	replicaset.applier.total = count;
>