[PATCH v2] iproto: close socket explicitly before wal_dir at exit

[PATCH v2] iproto: close socket explicitly before wal_dir at exit

[Ilya Kosarev]

From: Ilya Kosarev <ilyanapar@yandex.ru>

tarantool instance didn't close socket explicitly
which could cause hot standby instance to fail to bind
in case it tries to bind before socket is closed by OS.
Now it is fixed by closing socket explicitly before wal_dir.

Closes #3967
---
https://github.com/tarantool/tarantool/tree/i.kosarev/gh-3967-close-socket-explicitly
https://github.com/tarantool/tarantool/issues/3967

 src/box/box.cc    |  1 +
 src/box/iproto.cc | 13 +++++++++++++
 src/box/iproto.h  |  3 +++
 3 files changed, 17 insertions(+)

diff --git a/src/box/box.cc b/src/box/box.cc
index e7ec2891c..73d94f79b 100644
--- a/src/box/box.cc
+++ b/src/box/box.cc
@@ -1666,6 +1666,7 @@ box_free(void)
 		tuple_free();
 		port_free();
 #endif
+		iproto_free();
 		replication_free();
 		sequence_free();
 		gc_free();
diff --git a/src/box/iproto.cc b/src/box/iproto.cc
index bea35ce87..b1d24910f 100644
--- a/src/box/iproto.cc
+++ b/src/box/iproto.cc
@@ -2080,3 +2080,16 @@ iproto_set_msg_max(int new_iproto_msg_max)
 	iproto_do_cfg(&cfg_msg);
 	cpipe_set_max_input(&net_pipe, new_iproto_msg_max / 2);
 }
+
+void
+iproto_free()
+{
+	tt_pthread_cancel(net_cord.id);
+	tt_pthread_join(net_cord.id, NULL);
+	/*
+	* Close socket descriptor to prevent hot standby instance
+	* failing to bind in case it tries to bind
+	* before socket is closed by OS.
+	*/
+	close(binary.ev.fd);
+}
diff --git a/src/box/iproto.h b/src/box/iproto.h
index b9a6cf8f7..8f3607ffc 100644
--- a/src/box/iproto.h
+++ b/src/box/iproto.h
@@ -80,6 +80,9 @@ iproto_listen(const char *uri);
 void
 iproto_set_msg_max(int iproto_msg_max);
 
+void
+iproto_free();
+
 #endif /* defined(__cplusplus) */
 
 #endif
-- 
2.17.1

Re: [PATCH v2] iproto: close socket explicitly before wal_dir at exit

[Vladimir Davydov]

On Mon, Feb 25, 2019 at 05:56:25PM +0300, Ilya Kosarev wrote:
> From: Ilya Kosarev <ilyanapar@yandex.ru>
> 
> tarantool instance didn't close socket explicitly
> which could cause hot standby instance to fail to bind
> in case it tries to bind before socket is closed by OS.
> Now it is fixed by closing socket explicitly before wal_dir.
> 
> Closes #3967
> ---
> https://github.com/tarantool/tarantool/tree/i.kosarev/gh-3967-close-socket-explicitly
> https://github.com/tarantool/tarantool/issues/3967

Please write a brief change log whenever you resubmit a patch and bump
the version in the subject. For more details, see

https://tarantool.io/en/doc/1.10/dev_guide/developer_guidelines/#how-to-submit-a-patch-for-review

> 
>  src/box/box.cc    |  1 +
>  src/box/iproto.cc | 13 +++++++++++++
>  src/box/iproto.h  |  3 +++
>  3 files changed, 17 insertions(+)

I pushed the patch to 2.1 and 1.10. Thanks.

Re: [tarantool-patches] [PATCH v2] iproto: close socket explicitly before wal_dir at exit

[Konstantin Osipov]

* Ilya Kosarev <i.kosarev@tarantool.org> [19/02/25 19:14]:
> +void
> +iproto_free()
> +{
> +	tt_pthread_cancel(net_cord.id);
> +	tt_pthread_join(net_cord.id, NULL);
> +	/*
> +	* Close socket descriptor to prevent hot standby instance
> +	* failing to bind in case it tries to bind
> +	* before socket is closed by OS.
> +	*/
> +	close(binary.ev.fd);
> +}

What if there is no socket descriptor in binary.ev.fd?
I understand it's OK to close a non-existing descriptor, but still
looks a bit messy to me.

-- 
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.io - www.twitter.com/kostja_osipov

Re: [tarantool-patches] [PATCH v2] iproto: close socket explicitly before wal_dir at exit

[Vladimir Davydov]

On Mon, Feb 25, 2019 at 07:50:47PM +0300, Konstantin Osipov wrote:
> * Ilya Kosarev <i.kosarev@tarantool.org> [19/02/25 19:14]:
> > +void
> > +iproto_free()
> > +{
> > +	tt_pthread_cancel(net_cord.id);
> > +	tt_pthread_join(net_cord.id, NULL);
> > +	/*
> > +	* Close socket descriptor to prevent hot standby instance
> > +	* failing to bind in case it tries to bind
> > +	* before socket is closed by OS.
> > +	*/
> > +	close(binary.ev.fd);
> > +}
> 
> What if there is no socket descriptor in binary.ev.fd?
> I understand it's OK to close a non-existing descriptor, but still
> looks a bit messy to me.

Yeah, missed that, sorry. Fixed as shown below:

From 37b883145ff0464640d917fe58b488eb7d539400 Mon Sep 17 00:00:00 2001
From: Vladimir Davydov <vdavydov.dev@gmail.com>
Date: Mon, 25 Feb 2019 20:03:27 +0300
Subject: [PATCH] iproto: don't attempt to close socket if it was not open

This can't entail any consequences, because socket fd is set to -1 in
this case, but this just looks a bit messy. Let's clean it up.

Follow-up commit 305dbcf68953 ("iproto: close socket explicitly before
wal_dir at exit").

diff --git a/src/box/iproto.cc b/src/box/iproto.cc
index 226b9d06..863eb4f0 100644
--- a/src/box/iproto.cc
+++ b/src/box/iproto.cc
@@ -2152,5 +2152,6 @@ iproto_free()
 	* failing to bind in case it tries to bind before socket
 	* is closed by OS.
 	*/
-	close(binary.ev.fd);
+	if (evio_service_is_active(&binary))
+		close(binary.ev.fd);
 }

[Tarantool-patches] [PATCH v2] iproto: close socket explicitly before wal_dir at exit

[Ilya Kosarev]

From: Ilya Kosarev <ilyanapar@yandex.ru>

tarantool instance didn't close socket explicitly
which could cause hot standby instance to fail to bind
in case it tries to bind before socket is closed by OS.
Now it is fixed by closing socket explicitly before wal_dir.

Closes #3967
---
https://github.com/tarantool/tarantool/tree/i.kosarev/gh-3967-close-socket-explicitly
https://github.com/tarantool/tarantool/issues/3967

 src/box/box.cc    |  1 +
 src/box/iproto.cc | 13 +++++++++++++
 src/box/iproto.h  |  3 +++
 3 files changed, 17 insertions(+)

diff --git a/src/box/box.cc b/src/box/box.cc
index e7ec2891c..73d94f79b 100644
--- a/src/box/box.cc
+++ b/src/box/box.cc
@@ -1666,6 +1666,7 @@ box_free(void)
 		tuple_free();
 		port_free();
 #endif
+		iproto_free();
 		replication_free();
 		sequence_free();
 		gc_free();
diff --git a/src/box/iproto.cc b/src/box/iproto.cc
index bea35ce87..b1d24910f 100644
--- a/src/box/iproto.cc
+++ b/src/box/iproto.cc
@@ -2080,3 +2080,16 @@ iproto_set_msg_max(int new_iproto_msg_max)
 	iproto_do_cfg(&cfg_msg);
 	cpipe_set_max_input(&net_pipe, new_iproto_msg_max / 2);
 }
+
+void
+iproto_free()
+{
+	tt_pthread_cancel(net_cord.id);
+	tt_pthread_join(net_cord.id, NULL);
+	/*
+	* Close socket descriptor to prevent hot standby instance
+	* failing to bind in case it tries to bind
+	* before socket is closed by OS.
+	*/
+	close(binary.ev.fd);
+}
diff --git a/src/box/iproto.h b/src/box/iproto.h
index b9a6cf8f7..8f3607ffc 100644
--- a/src/box/iproto.h
+++ b/src/box/iproto.h
@@ -80,6 +80,9 @@ iproto_listen(const char *uri);
 void
 iproto_set_msg_max(int iproto_msg_max);
 
+void
+iproto_free();
+
 #endif /* defined(__cplusplus) */
 
 #endif
-- 
2.17.1