[tarantool-patches] Re: [PATCH 2/5] swim: introduce member reference API

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

On 01/05/2019 08:15, Konstantin Osipov wrote:
> * Vladislav Shpilevoy <v.shpilevoy@tarantool.org> [19/05/01 08:07]:
> 
> I disagree with adding a member status for dropped member.
> 
> By resetting the status you lose the information about the
> original status, as set when leaving the membership table.

My motivation of resetting the status was that a user can't
rely on that anymore, it is not valid. If a dropped member's
status is ALIVE, it does not mean, that it is really alive.
User's application should not use the old status.

And I wanted to force a user to stop looking at that invalid
member. Also new status allows to do not add a new function to
check member validity.

Please, reconsider this implementation again after my explanation
above. In case I did not persuade you, I implemented a
flag-based patch, see at the end of the mail.

> Anyway, member status is part of the swim protocol, and should not
> be used to reflect any memory management details specific to the
> programming language in use.
> 
> Please either have a separate status for zombie members 
> or simply check for refs == 1.

I thought about that, refs == 1 does not work. Consider this:
I do swim:find_member() 3 times in Lua. Now ref count of the
member is 4. Then the member is dropped. Its ref count == 3,
but it is dropped. So refs == 1 does not work as a flag.

I added a flag. But please, think again about status invalidation -
it can prevent fallacious belief in status validity in user space.

===================================================================

commit 0040b1966de9ca669676420a5e6c86aacd2f001e
Author: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Date:   Tue Apr 30 18:46:58 2019 +0300

    swim: introduce member reference API
    
    Struct swim_member pointer is used to learn member's status,
    payload, incarnation etc. To obtain a pointer, a one should
    either lookup the member by UUID, or obtain from iterators API.
    The former is long, the latter is useless when a point lookup is
    needed.
    
    On the other hand, it was not safe to keep struct swim_member
    pointer for a long time, because it could be deleted at any
    moment.
    
    This patch allows to reference a member and be sure that it will
    not be deleted until dereferenced explicitly. The member still
    can be dropped from the member table, but its memory will be
    valid. To detect that a member is dropped, a user can use
    swim_member_is_dropped() function.
    
    Part of #3234

diff --git a/src/lib/swim/swim.c b/src/lib/swim/swim.c
index dfee16493..ff95a8c2b 100644
--- a/src/lib/swim/swim.c
+++ b/src/lib/swim/swim.c
@@ -243,6 +243,19 @@ struct swim_member {
 	 * Position in a queue of members in the current round.
 	 */
 	struct rlist in_round_queue;
+	/**
+	 * Reference counter. Used by public API to prevent the
+	 * member deletion after it is obtained by UUID or from an
+	 * iterator.
+	 */
+	int refs;
+	/**
+	 * True, if the member was dropped from the member table.
+	 * At the same time it still can be not deleted, if users
+	 * of the public API referenced the member. Dropped member
+	 * is not valid anymore and should be dereferenced.
+	 */
+	bool is_dropped;
 	/**
 	 *
 	 *                 Dissemination component
@@ -638,11 +651,34 @@ swim_ping_task_complete(struct swim_task *task,
 	swim_wait_ack(swim, m, false);
 }
 
+void
+swim_member_ref(struct swim_member *member)
+{
+	++member->refs;
+}
+
+void
+swim_member_unref(struct swim_member *member)
+{
+	assert(member->refs > 0);
+	if (--member->refs == 0) {
+		free(member->payload);
+		free(member);
+	}
+}
+
+bool
+swim_member_is_dropped(const struct swim_member *member)
+{
+	return member->is_dropped;
+}
+
 /** Free member's resources. */
 static inline void
 swim_member_delete(struct swim_member *member)
 {
 	assert(rlist_empty(&member->in_round_queue));
+	member->is_dropped = true;
 
 	/* Failure detection component. */
 	assert(heap_node_is_stray(&member->in_wait_ack_heap));
@@ -651,9 +687,8 @@ swim_member_delete(struct swim_member *member)
 
 	/* Dissemination component. */
 	assert(rlist_empty(&member->in_dissemination_queue));
-	free(member->payload);
 
-	free(member);
+	swim_member_unref(member);
 }
 
 /** Create a new member. It is not registered anywhere here. */
@@ -667,6 +702,7 @@ swim_member_new(const struct sockaddr_in *addr, const struct tt_uuid *uuid,
 		diag_set(OutOfMemory, sizeof(*member), "calloc", "member");
 		return NULL;
 	}
+	member->refs = 1;
 	member->status = status;
 	member->addr = *addr;
 	member->uuid = *uuid;
@@ -1969,14 +2005,14 @@ swim_quit(struct swim *swim)
 	swim_quit_step_complete(task, &swim->scheduler, 0);
 }
 
-const struct swim_member *
+struct swim_member *
 swim_self(struct swim *swim)
 {
 	assert(swim_is_configured(swim));
 	return swim->self;
 }
 
-const struct swim_member *
+struct swim_member *
 swim_member_by_uuid(struct swim *swim, const struct tt_uuid *uuid)
 {
 	assert(swim_is_configured(swim));
@@ -1997,7 +2033,7 @@ swim_iterator_open(struct swim *swim)
 	return (struct swim_iterator *) swim;
 }
 
-const struct swim_member *
+struct swim_member *
 swim_iterator_next(struct swim_iterator *iterator)
 {
 	struct swim *swim = (struct swim *) iterator;
diff --git a/src/lib/swim/swim.h b/src/lib/swim/swim.h
index 653e45be7..2f53716c7 100644
--- a/src/lib/swim/swim.h
+++ b/src/lib/swim/swim.h
@@ -186,7 +186,7 @@ void
 swim_quit(struct swim *swim);
 
 /** Get a SWIM member, describing this instance. */
-const struct swim_member *
+struct swim_member *
 swim_self(struct swim *swim);
 
 /**
@@ -194,7 +194,7 @@ swim_self(struct swim *swim);
  * @retval NULL Not found.
  * @retval not NULL A member.
  */
-const struct swim_member *
+struct swim_member *
 swim_member_by_uuid(struct swim *swim, const struct tt_uuid *uuid);
 
 /** Member's current status. */
@@ -216,7 +216,7 @@ swim_iterator_open(struct swim *swim);
  * @retval NULL EOF.
  * @retval not NULL A valid member.
  */
-const struct swim_member *
+struct swim_member *
 swim_iterator_next(struct swim_iterator *iterator);
 
 /** Close an iterator. */
@@ -239,6 +239,28 @@ swim_member_incarnation(const struct swim_member *member);
 const char *
 swim_member_payload(const struct swim_member *member, uint16_t *size);
 
+/**
+ * Reference a member. The member memory will be valid until unref
+ * is called.
+ */
+void
+swim_member_ref(struct swim_member *member);
+
+/**
+ * Dereference a member. After that it may be deleted and can't be
+ * accessed anymore.
+ */
+void
+swim_member_unref(struct swim_member *member);
+
+/**
+ * Check if a member was dropped from the member table. It means,
+ * that the member is not valid anymore and should be
+ * dereferenced.
+ */
+bool
+swim_member_is_dropped(const struct swim_member *member);
+
 #if defined(__cplusplus)
 }
 #endif
diff --git a/test/unit/swim.c b/test/unit/swim.c
index 5aaae089d..8303e2900 100644
--- a/test/unit/swim.c
+++ b/test/unit/swim.c
@@ -176,7 +176,7 @@ swim_test_cfg(void)
 static void
 swim_test_add_remove(void)
 {
-	swim_start_test(13);
+	swim_start_test(14);
 
 	struct swim_cluster *cluster = swim_cluster_new(2);
 	swim_cluster_add_link(cluster, 0, 1);
@@ -232,6 +232,17 @@ swim_test_add_remove(void)
 	swim_cluster_unblock_io(cluster, 0);
 	is(swim_cluster_wait_fullmesh(cluster, 1), 0,
 	   "back in fullmesh after a member removal in the middle of a step");
+	/*
+	 * Check that member removal does not delete a member,
+	 * only unrefs.
+	 */
+	const struct tt_uuid *s1_uuid = swim_member_uuid(swim_self(s1));
+	struct swim_member *s1_view = swim_member_by_uuid(s2, s1_uuid);
+	swim_member_ref(s1_view);
+	swim_remove_member(s2, s1_uuid);
+	ok(swim_member_is_dropped(s1_view), "if a referenced "\
+	   "member is dropped, it can be detected from the public API");
+	swim_member_unref(s1_view);
 
 	swim_cluster_delete(cluster);
 
diff --git a/test/unit/swim.result b/test/unit/swim.result
index 6e439541a..24597641c 100644
--- a/test/unit/swim.result
+++ b/test/unit/swim.result
@@ -47,7 +47,7 @@ ok 3 - subtests
 ok 4 - subtests
 	*** swim_test_cfg: done ***
 	*** swim_test_add_remove ***
-    1..13
+    1..14
     ok 1 - can not add an existing member
     ok 2 - diag says 'already exists'
     ok 3 - can not add a invalid uri
@@ -61,6 +61,7 @@ ok 4 - subtests
     ok 11 - after removal the cluster is not in fullmesh
     ok 12 - but it is back in 1 step
     ok 13 - back in fullmesh after a member removal in the middle of a step
+    ok 14 - if a referenced member is dropped, it can be detected from the public API
 ok 5 - subtests
 	*** swim_test_add_remove: done ***
 	*** swim_test_basic_failure_detection ***