From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id B16D925CCC for ; Thu, 2 May 2019 11:10:07 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id pMsNXz_xykCb for ; Thu, 2 May 2019 11:10:07 -0400 (EDT) Received: from smtp37.i.mail.ru (smtp37.i.mail.ru [94.100.177.97]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id A93F8285E0 for ; Thu, 2 May 2019 11:10:06 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH 2/5] swim: introduce member reference API References: <93b1da3130537b7182f0326e9ede8202a1319906.1556663421.git.v.shpilevoy@tarantool.org> <20190501051515.GB1139@atlas> From: Vladislav Shpilevoy Message-ID: <65df3d66-34cf-b339-3645-e16a74517c30@tarantool.org> Date: Thu, 2 May 2019 18:10:01 +0300 MIME-Version: 1.0 In-Reply-To: <20190501051515.GB1139@atlas> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-Help: List-Unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-Subscribe: List-Owner: List-post: List-Archive: To: tarantool-patches@freelists.org, Konstantin Osipov On 01/05/2019 08:15, Konstantin Osipov wrote: > * Vladislav Shpilevoy [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 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 ***