[tarantool-patches] [PATCH] CBUS: fix cbus_unpair behavior

[tarantool-patches] [PATCH] CBUS: fix cbus_unpair behavior

[Georgy Kirichenko]

Each side should close outgoing cpipe while cbus unpair processing.

Fixes: #4484
---
Branch: https://github.com/tarantool/tarantool/tree/g.kirichenko/gh-4484-cbus-unpair-cpipe-destroy
Issue: https://github.com/tarantool/tarantool/issues/4484
 src/lib/core/cbus.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/src/lib/core/cbus.c b/src/lib/core/cbus.c
index b3b1280e7..78f3e5341 100644
--- a/src/lib/core/cbus.c
+++ b/src/lib/core/cbus.c
@@ -547,6 +547,7 @@ struct cbus_unpair_msg {
 	void (*unpair_cb)(void *);
 	void *unpair_arg;
 	struct cpipe *src_pipe;
+	struct cpipe *dest_pipe;
 	bool complete;
 	struct fiber_cond cond;
 };
@@ -579,7 +580,7 @@ cbus_unpair_perform(struct cmsg *cmsg)
 	};
 	cmsg_init(cmsg, route);
 	cpipe_push(msg->src_pipe, cmsg);
-	cpipe_destroy(msg->src_pipe);
+	cpipe_destroy(msg->dest_pipe);
 }
 
 static void
@@ -607,6 +608,7 @@ cbus_unpair(struct cpipe *dest_pipe, struct cpipe *src_pipe,
 	msg.unpair_cb = unpair_cb;
 	msg.unpair_arg = unpair_arg;
 	msg.src_pipe = src_pipe;
+	msg.dest_pipe = dest_pipe;
 	msg.complete = false;
 	fiber_cond_create(&msg.cond);
 
@@ -621,7 +623,7 @@ cbus_unpair(struct cpipe *dest_pipe, struct cpipe *src_pipe,
 		fiber_cond_wait(&msg.cond);
 	}
 
-	cpipe_destroy(dest_pipe);
+	cpipe_destroy(src_pipe);
 }
 
 void
-- 
2.23.0

[tarantool-patches] Re: [PATCH] CBUS: fix cbus_unpair behavior

[Vladislav Shpilevoy]

Hi! Thanks for the patch! See 3 comments below.

1. We don't normally use uppercase in commit titles. Please, use 'cbus',
not 'CBUS'.

On 10/09/2019 16:17, Georgy Kirichenko wrote:
> Each side should close outgoing cpipe while cbus unpair processing.

2. Why? I didn't understand it. Please, explain in more details.

As I see in the code, cbus_pair() does cpipe_create(dest_pipe) in
the caller thread, and cbus_pair_perform() does cpipe_create(msg->src_pipe)
in the target thread.

On the other hand cbus_unpair() does cpipe_destroy(dest_pipe) and
cbus_unpair_perform() does cpipe_destroy(msg->src_pipe). Looks
symmetrical. What was wrong here?

> 
> Fixes: #4484
> ---
> Branch: https://github.com/tarantool/tarantool/tree/g.kirichenko/gh-4484-cbus-unpair-cpipe-destroy
> Issue: https://github.com/tarantool/tarantool/issues/4484
>  src/lib/core/cbus.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/src/lib/core/cbus.c b/src/lib/core/cbus.c
> index b3b1280e7..78f3e5341 100644
> --- a/src/lib/core/cbus.c
> +++ b/src/lib/core/cbus.c
> @@ -547,6 +547,7 @@ struct cbus_unpair_msg {
>  	void (*unpair_cb)(void *);
>  	void *unpair_arg;
>  	struct cpipe *src_pipe;
> +	struct cpipe *dest_pipe;

3. Please, use 'dst'. When we have 'src', for
destination we conventionally use 'dst'.

>  	bool complete;
>  	struct fiber_cond cond;
>  };

[tarantool-patches] Re: [PATCH] CBUS: fix cbus_unpair behavior

[Vladislav Shpilevoy]

And please, provide a test.