From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <v.shpilevoy@tarantool.org>
Received: from smtp44.i.mail.ru (smtp44.i.mail.ru [94.100.177.104])
 (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))
 (No client certificate requested)
 by dev.tarantool.org (Postfix) with ESMTPS id CBECD46970F
 for <tarantool-patches@dev.tarantool.org>;
 Sun, 24 Nov 2019 18:54:05 +0300 (MSK)
References: <cover.1574545667.git.i.kosarev@tarantool.org>
 <66ff4c73f329f83b616eeadbf5ea6f26994ecb4a.1574545667.git.i.kosarev@tarantool.org>
From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Message-ID: <8b2b2757-8926-1fbc-3f2b-1a1afea6fa52@tarantool.org>
Date: Sun, 24 Nov 2019 16:54:03 +0100
MIME-Version: 1.0
In-Reply-To: <66ff4c73f329f83b616eeadbf5ea6f26994ecb4a.1574545667.git.i.kosarev@tarantool.org>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Subject: Re: [Tarantool-patches] [PATCH v2 2/4] replication: fix appliers
 pruning
List-Id: Tarantool development patches <tarantool-patches.dev.tarantool.org>
List-Unsubscribe: <https://lists.tarantool.org/mailman/options/tarantool-patches>, 
 <mailto:tarantool-patches-request@dev.tarantool.org?subject=unsubscribe>
List-Archive: <https://lists.tarantool.org/pipermail/tarantool-patches/>
List-Post: <mailto:tarantool-patches@dev.tarantool.org>
List-Help: <mailto:tarantool-patches-request@dev.tarantool.org?subject=help>
List-Subscribe: <https://lists.tarantool.org/mailman/listinfo/tarantool-patches>, 
 <mailto:tarantool-patches-request@dev.tarantool.org?subject=subscribe>
To: Ilya Kosarev <i.kosarev@tarantool.org>, tarantool-patches@dev.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;
>