Tarantool development patches archive
 help / color / mirror / Atom feed
From: Vladimir Davydov <vdavydov.dev@gmail.com>
To: Ilya Kosarev <i.kosarev@tarantool.org>
Cc: tarantool-patches@freelists.org, georgy@tarantool.org,
	i.kosarev@corp.mail.ru
Subject: Re: [tarantool-patches] [PATCH] Close socket explicitly before wal_dir at exit
Date: Mon, 25 Feb 2019 14:28:11 +0300	[thread overview]
Message-ID: <20190225112810.3ullwvxfhmsrph7o@esperanza> (raw)
In-Reply-To: <20190222120500.21180-1-i.kosarev@tarantool.org>

On Fri, Feb 22, 2019 at 03:05:00PM +0300, Ilya Kosarev wrote:
> 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.

Please keep the description text within 72 characters (it's a common
practice for git commit messages). Also, the subject line should be
prefixed with the subsystem name if applicable. In this particular case
it should be "iproto":

	iproto: close socket explicitly before wal_dir at exit

> 
> Closes #3967

This line is missing in the commit on the branch. Please fix.

> ---
> Branch: https://github.com/tarantool/tarantool/tree/i.kosarev/gh-3967-close-socket-explicitly
> Issue: https://github.com/tarantool/tarantool/issues/3967

"Branch" and "Issue" prefixes are useless - it's obvious which is which
without them. You could just add the two links.

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

It's unfortunate there's no test, but I couldn't reproduce the issue
either so I guess it's OK.

> 
> diff --git a/src/box/box.cc b/src/box/box.cc
> index 9a8ca1585..38bdebeba 100644
> --- a/src/box/box.cc
> +++ b/src/box/box.cc
> @@ -1666,6 +1666,7 @@ box_free(void)
>  		tuple_free();
>  		port_free();
>  #endif
> +		iproto_socket_free();
>  		replication_free();
>  		sequence_free();
>  		gc_free();
> diff --git a/src/box/iproto.cc b/src/box/iproto.cc
> index 19a945d2a..1d2fb3bdd 100644
> --- a/src/box/iproto.cc
> +++ b/src/box/iproto.cc
> @@ -2075,3 +2075,10 @@ 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_socket_free()

We typically call a subsystem constructor and destructor <subsys>_init
and <subsys>_free, respectively. Let's rename to iproto_free() please.

> +{
> +	tt_pthread_cancel(net_cord.id);

I think we should wait for the thread to exit before closing the fd,
otherwise it may spit an error message to the log. Take a look at how
replication_free does that, for instance.

Also, I think it's worth adding a comment to the code why it's important
to close the fd on shutdown.

> +	close(binary.ev.fd);
> +}

      reply	other threads:[~2019-02-25 11:28 UTC|newest]

Thread overview: 2+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-02-22 12:05 Ilya Kosarev
2019-02-25 11:28 ` Vladimir Davydov [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190225112810.3ullwvxfhmsrph7o@esperanza \
    --to=vdavydov.dev@gmail.com \
    --cc=georgy@tarantool.org \
    --cc=i.kosarev@corp.mail.ru \
    --cc=i.kosarev@tarantool.org \
    --cc=tarantool-patches@freelists.org \
    --subject='Re: [tarantool-patches] [PATCH] Close socket explicitly before wal_dir at exit' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox