[Tarantool-patches] [PATCH v6 0/3] iproto: greeting enhancement

[Tarantool-patches] [PATCH v6 0/3] iproto: greeting enhancement

[Ilya Kosarev]

These patches move greeting from tx thread to iproto and introduce
minor improvements into iproto logic.

Branch: https://github.com/tarantool/tarantool/tree/i.kosarev/gh-3776-handling-connections-in-iproto
Issue: https://github.com/tarantool/tarantool/issues/3776

Changes in v2:
 - docbot request provided
 - ChangeLog provided
 - net_send_msg() used instead of net_send_error() where needed
 - error cases made clear in iproto_msg_decode()
 - replaced net_check_connection() with iproto_connection_fail() predicate
 - improved tx session initialization fail processing to avoid extra tries
 - fixed obuf_dup() fail processing in iproto_process_connect()
 - added some comments
 - some comments style fixed

Changes in v3:
 - added braces to the confusing one-line if statement
 - updated ChangeLog
 - added description for iproto_msg_decode()
 - simplified error processing in iproto_msg_decode()

Changes in v4:
 - fixed msg->close_connection initialization bug
 - fixed false-sharing problem in iproto_connection struct
 - added needed assertion
 - added needed comments
 - names refactoring
 - simplified patch a bit: removed extra return value, extra 
 
Changes in v5:
 - reworked to avoid lazy initialization and extra changes

Changes in v6:
 - some changes are picket out into separate commits
 - main commit message rewrited to become more comprehensive
 - removed some extra changes
 - style fixes
 - greeting allocation fixed

Ilya Kosarev (3):
  iproto: move msg fields initialization to iproto_msg_new()
  iproto: fix comment and add assert on destruction
  iproto: move greeting from tx thread to iproto

 src/box/iproto.cc                             | 125 +++++++++++++-----
 test/app/gh-4787-netbox-empty-errmsg.result   |  18 ---
 test/app/gh-4787-netbox-empty-errmsg.test.lua |   8 --
 3 files changed, 92 insertions(+), 59 deletions(-)

-- 
2.17.1

[Tarantool-patches] [PATCH v6 1/3] iproto: move msg fields initialization to iproto_msg_new()

[Ilya Kosarev]

msg->close_connection flag was only initialized in iproto_on_accept()
while other struct iproto_msg fields are being initialized in
iproto_msg_new(). It is potentially dangerous for new logic involving
msg->close_connection flag, so it is now moved to iproto_msg_new().

Part of #3776
---
 src/box/iproto.cc | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/box/iproto.cc b/src/box/iproto.cc
index b8f65e5eca..6a1e509225 100644
--- a/src/box/iproto.cc
+++ b/src/box/iproto.cc
@@ -565,6 +565,7 @@ iproto_msg_new(struct iproto_connection *con)
 			 "connection %s", sio_socketname(con->input.fd));
 		return NULL;
 	}
+	msg->close_connection = false;
 	msg->connection = con;
 	rmean_collect(rmean_net, IPROTO_REQUESTS, 1);
 	return msg;
@@ -2040,7 +2041,6 @@ iproto_on_accept(struct evio_service * /* service */, int fd,
 	cmsg_init(&msg->base, connect_route);
 	msg->p_ibuf = con->p_ibuf;
 	msg->wpos = con->wpos;
-	msg->close_connection = false;
 	cpipe_push(&tx_pipe, &msg->base);
 	return 0;
 }
-- 
2.17.1

[Tarantool-patches] [PATCH v6 2/3] iproto: fix comment and add assert on destruction

[Ilya Kosarev]

The comment in tx_process_destroy() about obuf destroting was wrong.
Memory for them is actually allocated from tx-belonging slab cache and
tx_process_destroy() obviously happens in tx, so the comment is fixed
to reflect the reality.
It is also implied that connection is in IPROTO_CONNECTION_DESTROYED
state in tx_process_destroy(). Now it is verified with assert().

Part of #3776
---
 src/box/iproto.cc | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/src/box/iproto.cc b/src/box/iproto.cc
index 6a1e509225..f7330af21d 100644
--- a/src/box/iproto.cc
+++ b/src/box/iproto.cc
@@ -1379,13 +1379,14 @@ tx_process_destroy(struct cmsg *m)
 {
 	struct iproto_connection *con =
 		container_of(m, struct iproto_connection, destroy_msg);
+	assert(con->state == IPROTO_CONNECTION_DESTROYED);
 	if (con->session) {
 		session_destroy(con->session);
 		con->session = NULL; /* safety */
 	}
 	/*
-	 * Got to be done in iproto thread since
-	 * that's where the memory is allocated.
+	 * obuf is being destroyed in tx thread cause it is where
+	 * it was allocated.
 	 */
 	obuf_destroy(&con->obuf[0]);
 	obuf_destroy(&con->obuf[1]);
-- 
2.17.1

[Tarantool-patches] [PATCH v6 3/3] iproto: move greeting from tx thread to iproto

[Ilya Kosarev]

On connection, an evio service callback is invoked to accept it. The
next step after acceptance was to process connection to tx thread
through cbus. This meant that any connection interaction involves tx
thread even before we get to send the greeting to the client.
Consequently, the client might reach the instance with enormous number
of connections, leading to the file descriptors limit exhaustion in
case of unresponsive tx thread (for example, when building secondary
index) and extra tx_process_connects afterwords, even in case the
instance doesn't fail with too many open files error.
This patch allows iproto to accept connection and send greeting by
itself. Thus the connection is being established and stays in
fetch_schema state while tx thread is unresponsive.

Closes #3776
---
 src/box/iproto.cc                             | 118 +++++++++++++-----
 test/app/gh-4787-netbox-empty-errmsg.result   |  18 ---
 test/app/gh-4787-netbox-empty-errmsg.test.lua |   8 --
 3 files changed, 88 insertions(+), 56 deletions(-)

diff --git a/src/box/iproto.cc b/src/box/iproto.cc
index f7330af21d..a7da115925 100644
--- a/src/box/iproto.cc
+++ b/src/box/iproto.cc
@@ -453,6 +453,8 @@ struct iproto_connection
 	 * meaningless.
 	 */
 	size_t parse_size;
+	/** Iproto buffer used to send greeting. */
+	char greeting_buf[IPROTO_GREETING_SIZE];
 	/**
 	 * Nubmer of active long polling requests that have already
 	 * discarded their arguments in order not to stall other
@@ -1091,6 +1093,46 @@ iproto_connection_on_output(ev_loop *loop, struct ev_io *watcher,
 	}
 }
 
+static int
+iproto_buf_flush(struct iproto_connection *con)
+{
+	struct iovec greeting;
+	greeting.iov_base = &con->greeting_buf;
+	greeting.iov_len = IPROTO_GREETING_SIZE;
+	ssize_t nwr = sio_writev(con->output.fd, &greeting, 1);
+
+	if (nwr > 0) {
+		/* Count statistics */
+		rmean_collect(rmean_net, IPROTO_SENT, nwr);
+		return 1;
+	} else if (nwr < 0 && !sio_wouldblock(errno)) {
+		return -1;
+	}
+
+	return 0;
+}
+
+static void
+iproto_connection_on_greeting(ev_loop *loop, struct ev_io *watcher,
+			      int /* revents */)
+{
+	struct iproto_connection *con =
+		(struct iproto_connection *)watcher->data;
+	int rc = iproto_buf_flush(con);
+	if (rc <= 0) {
+		if (rc == 0) {
+			ev_io_start(loop, &con->output);
+		} else {
+			diag_log();
+			con->state = IPROTO_CONNECTION_CLOSED;
+		}
+		return;
+	}
+	ev_io_stop(con->loop, &con->output);
+	ev_io_init(&con->output, iproto_connection_on_output,
+		   con->output.fd, EV_WRITE);
+}
+
 static struct iproto_connection *
 iproto_connection_new(int fd)
 {
@@ -1103,7 +1145,7 @@ iproto_connection_new(int fd)
 	con->input.data = con->output.data = con;
 	con->loop = loop();
 	ev_io_init(&con->input, iproto_connection_on_input, fd, EV_READ);
-	ev_io_init(&con->output, iproto_connection_on_output, fd, EV_WRITE);
+	ev_io_init(&con->output, iproto_connection_on_greeting, fd, EV_WRITE);
 	ibuf_create(&con->ibuf[0], cord_slab_cache(), iproto_readahead);
 	ibuf_create(&con->ibuf[1], cord_slab_cache(), iproto_readahead);
 	obuf_create(&con->obuf[0], &net_slabc, iproto_readahead);
@@ -1938,50 +1980,64 @@ net_end_subscribe(struct cmsg *m)
 }
 
 /**
- * Handshake a connection: invoke the on-connect trigger
- * and possibly authenticate. Try to send the client an error
- * upon a failure.
+ * Handshake a connection: send greeting for it.
+ */
+static void
+iproto_process_connect(struct iproto_msg *msg)
+{
+	struct iproto_connection *con = msg->connection;
+	/*
+	 * INSTANCE_UUID is guaranteed to be inited before this moment.
+	 * We start listening either in local_recovery() or bootstrap().
+	 * The INSTANCE_UUID is ensured to be inited in the beginning of
+	 * both methods. In case of local_recovery() it is verified that
+	 * INSTANCE_UUID was read from the snapshot in memtx_engine_new().
+	 * In bootstrap() INSTANCE_UUID is either taken from the
+	 * instance_uuid box.cfg{} param or created on the spot.
+	 */
+	random_bytes(con->salt, IPROTO_SALT_SIZE);
+	greeting_encode(con->greeting_buf, tarantool_version_id(),
+			&INSTANCE_UUID, con->salt, IPROTO_SALT_SIZE);
+	assert(evio_has_fd(&con->output));
+	ev_feed_event(con->loop, &con->output, EV_WRITE);
+}
+
+/**
+ * Create the session and invoke the on_connect triggers.
  */
 static void
 tx_process_connect(struct cmsg *m)
 {
 	struct iproto_msg *msg = (struct iproto_msg *) m;
 	struct iproto_connection *con = msg->connection;
-	struct obuf *out = msg->connection->tx.p_obuf;
-	try {              /* connect. */
-		con->session = session_create(SESSION_TYPE_BINARY);
-		if (con->session == NULL)
-			diag_raise();
-		con->session->meta.connection = con;
-		tx_fiber_init(con->session, 0);
-		char *greeting = (char *) static_alloc(IPROTO_GREETING_SIZE);
-		/* TODO: dirty read from tx thread */
-		struct tt_uuid uuid = INSTANCE_UUID;
-		random_bytes(con->salt, IPROTO_SALT_SIZE);
-		greeting_encode(greeting, tarantool_version_id(), &uuid,
-				con->salt, IPROTO_SALT_SIZE);
-		obuf_dup_xc(out, greeting, IPROTO_GREETING_SIZE);
-		if (! rlist_empty(&session_on_connect)) {
-			if (session_run_on_connect_triggers(con->session) != 0)
-				diag_raise();
-		}
-		iproto_wpos_create(&msg->wpos, out);
-	} catch (Exception *e) {
+
+	con->session = session_create(SESSION_TYPE_BINARY);
+	if (con->session == NULL) {
 		tx_reply_error(msg);
 		msg->close_connection = true;
+		return;
+	}
+	con->session->meta.connection = con;
+
+	tx_fiber_init(con->session, 0);
+	if (! rlist_empty(&session_on_connect)) {
+		if (session_run_on_connect_triggers(con->session) != 0) {
+			tx_reply_error(msg);
+			msg->close_connection = true;
+		}
 	}
 }
 
 /**
- * Send a response to connect to the client or close the
- * connection in case on_connect trigger failed.
+ * Try to send the client an error upon a failure. Start reading
+ * input in case the connection is inited and all good.
  */
 static void
-net_send_greeting(struct cmsg *m)
+net_finish_connect(struct cmsg *m)
 {
 	struct iproto_msg *msg = (struct iproto_msg *) m;
 	struct iproto_connection *con = msg->connection;
-	if (msg->close_connection) {
+	if (msg->close_connection || con->state == IPROTO_CONNECTION_CLOSED) {
 		struct obuf *out = msg->wpos.obuf;
 		int64_t nwr = sio_writev(con->output.fd, out->iov,
 					 obuf_iovcnt(out));
@@ -1993,6 +2049,7 @@ net_send_greeting(struct cmsg *m)
 			diag_log();
 		}
 		assert(iproto_connection_is_idle(con));
+		con->state = IPROTO_CONNECTION_ALIVE;
 		iproto_connection_close(con);
 		iproto_msg_delete(msg);
 		return;
@@ -2005,13 +2062,13 @@ net_send_greeting(struct cmsg *m)
 	 */
 	assert(evio_has_fd(&con->output));
 	/* Handshake OK, start reading input. */
-	ev_feed_event(con->loop, &con->output, EV_WRITE);
+	ev_feed_event(con->loop, &con->input, EV_READ);
 	iproto_msg_delete(msg);
 }
 
 static const struct cmsg_hop connect_route[] = {
 	{ tx_process_connect, &net_pipe },
-	{ net_send_greeting, NULL },
+	{ net_finish_connect, NULL },
 };
 
 /** }}} */
@@ -2042,6 +2099,7 @@ iproto_on_accept(struct evio_service * /* service */, int fd,
 	cmsg_init(&msg->base, connect_route);
 	msg->p_ibuf = con->p_ibuf;
 	msg->wpos = con->wpos;
+	iproto_process_connect(msg);
 	cpipe_push(&tx_pipe, &msg->base);
 	return 0;
 }
diff --git a/test/app/gh-4787-netbox-empty-errmsg.result b/test/app/gh-4787-netbox-empty-errmsg.result
index d30337a050..6389b27bc8 100644
--- a/test/app/gh-4787-netbox-empty-errmsg.result
+++ b/test/app/gh-4787-netbox-empty-errmsg.result
@@ -38,24 +38,6 @@ req_during_auth()
  | - Connection is not established, state is "auth"
  | ...
 
--- Check the same for 'initial' state.
-ok, err = nil
- | ---
- | ...
-do                                                                              \
-    c = netbox.connect(box.cfg.listen, {wait_connected = false})                \
-    ok, err = pcall(c.call, c, 'echo', {}, {is_async = true})                   \
-end
- | ---
- | ...
-ok, err
- | ---
- | - false
- | - Connection is not established, state is "initial"
- | ...
-c:close()
- | ---
- | ...
 box.schema.user.drop('test')
  | ---
  | ...
diff --git a/test/app/gh-4787-netbox-empty-errmsg.test.lua b/test/app/gh-4787-netbox-empty-errmsg.test.lua
index 0eecaa1bf0..55ea43f26f 100755
--- a/test/app/gh-4787-netbox-empty-errmsg.test.lua
+++ b/test/app/gh-4787-netbox-empty-errmsg.test.lua
@@ -21,12 +21,4 @@ end
 
 req_during_auth()
 
--- Check the same for 'initial' state.
-ok, err = nil
-do                                                                              \
-    c = netbox.connect(box.cfg.listen, {wait_connected = false})                \
-    ok, err = pcall(c.call, c, 'echo', {}, {is_async = true})                   \
-end
-ok, err
-c:close()
 box.schema.user.drop('test')
-- 
2.17.1

Re: [Tarantool-patches] [PATCH v6 3/3] iproto: move greeting from tx thread to iproto

[Vladislav Shpilevoy]

Hi! Thanks for the patch!

See 4 comments below.

On 13.12.2020 00:38, Ilya Kosarev wrote:
> On connection, an evio service callback is invoked to accept it. The
> next step after acceptance was to process connection to tx thread
> through cbus. This meant that any connection interaction involves tx
> thread even before we get to send the greeting to the client.
> Consequently, the client might reach the instance with enormous number
> of connections, leading to the file descriptors limit exhaustion in
> case of unresponsive tx thread (for example, when building secondary
> index) and extra tx_process_connects afterwords, even in case the
> instance doesn't fail with too many open files error.
> This patch allows iproto to accept connection and send greeting by
> itself. Thus the connection is being established and stays in
> fetch_schema state while tx thread is unresponsive.
> 
> Closes #3776
> ---
>  src/box/iproto.cc                             | 118 +++++++++++++-----
>  test/app/gh-4787-netbox-empty-errmsg.result   |  18 ---
>  test/app/gh-4787-netbox-empty-errmsg.test.lua |   8 --
>  3 files changed, 88 insertions(+), 56 deletions(-)
> 
> diff --git a/src/box/iproto.cc b/src/box/iproto.cc
> index f7330af21d..a7da115925 100644
> --- a/src/box/iproto.cc
> +++ b/src/box/iproto.cc
> @@ -1091,6 +1093,46 @@ iproto_connection_on_output(ev_loop *loop, struct ev_io *watcher,
>  	}
>  }
>  
> +static int
> +iproto_buf_flush(struct iproto_connection *con)

1. The function lacks a comment. It is not so trivial why do we
have 2 flush functions. Also maybe worth renaming it to
iproto_connection_greeting_flush. Because it is a method of
'iproto_connection' (thus the prefix), and works with the
greeting only.

> +{
> +	struct iovec greeting;
> +	greeting.iov_base = &con->greeting_buf;
> +	greeting.iov_len = IPROTO_GREETING_SIZE;
> +	ssize_t nwr = sio_writev(con->output.fd, &greeting, 1);
> +
> +	if (nwr > 0) {
> +		/* Count statistics */
> +		rmean_collect(rmean_net, IPROTO_SENT, nwr);
> +		return 1;
> +	} else if (nwr < 0 && !sio_wouldblock(errno)) {
> +		return -1;
> +	}
> +
> +	return 0;
> +}
> +
> +static void
> +iproto_connection_on_greeting(ev_loop *loop, struct ev_io *watcher,
> +			      int /* revents */)
> +{
> +	struct iproto_connection *con =
> +		(struct iproto_connection *)watcher->data;
> +	int rc = iproto_buf_flush(con);
> +	if (rc <= 0) {
> +		if (rc == 0) {
> +			ev_io_start(loop, &con->output);
> +		} else {
> +			diag_log();
> +			con->state = IPROTO_CONNECTION_CLOSED;

2. I don't understand how this part works. You didn't close the
socket, and didn't delete it from libev. But IPROTO_CONNECTION_CLOSED
clearly states

	/**
	 * Socket was closed, a notification is sent to the TX
	 * thread to close the session.
	 */

The way you change it here does not match the description. AFAIU,
the only legal function able to set this state is
iproto_connection_close(), and it should remain so.

Moreover, I see this should crash in an assertion in
iproto_connection_close(). Because this function will be called by
net_finish_connect() due to state being CLOSED (which is weird -
why is close() called if the state is already CLOSED?). But inside
it has "assert(con->state == IPROTO_CONNECTION_ALIVE)" in case the
ev_watcher still has a descriptor - it still has, so it will go there
any crash.

Did I miss something?

> +		}
> +		return;
> +	}
> +	ev_io_stop(con->loop, &con->output);
> +	ev_io_init(&con->output, iproto_connection_on_output,
> +		   con->output.fd, EV_WRITE);
> +}

3. What if sio_writev() has written only half of the bytes?
You didn't check and immediately proceeded to the normal
operation mode. But the greeting wasn't sent yet. Not all.

Also you don't propagate iov_base in case only a part of
the data was sent.

You can easily check it using a temporary error injection.
Before sio_writev() try to inject iov_len =
max(iov_len, IPROTO_GREETING_SIZE / 2) to force it to work
in 2 iterations, and you will see the issue.

Partial write is a rare thing, but it is totally real. It
is not some kind of an impossible thing only from the books.
Even if it is just 128 bytes.

> +
>  static struct iproto_connection *
>  iproto_connection_new(int fd)
>  {
> @@ -1938,50 +1980,64 @@ net_end_subscribe(struct cmsg *m)
>  }
>  
>  /**
> - * Handshake a connection: invoke the on-connect trigger
> - * and possibly authenticate. Try to send the client an error
> - * upon a failure.
> + * Handshake a connection: send greeting for it.
> + */
> +static void
> +iproto_process_connect(struct iproto_msg *msg)

4. Why do you take iproto_msg object if the only thing you do
with it is take the connection object? Wouldn't it be simpler
and more understandable, if it would be iproto_connection_start()
or something like that? With an iproto_connection * argument.

> +{
> +	struct iproto_connection *con = msg->connection;
> +	/*
> +	 * INSTANCE_UUID is guaranteed to be inited before this moment.
> +	 * We start listening either in local_recovery() or bootstrap().
> +	 * The INSTANCE_UUID is ensured to be inited in the beginning of
> +	 * both methods. In case of local_recovery() it is verified that
> +	 * INSTANCE_UUID was read from the snapshot in memtx_engine_new().
> +	 * In bootstrap() INSTANCE_UUID is either taken from the
> +	 * instance_uuid box.cfg{} param or created on the spot.
> +	 */
> +	random_bytes(con->salt, IPROTO_SALT_SIZE);
> +	greeting_encode(con->greeting_buf, tarantool_version_id(),
> +			&INSTANCE_UUID, con->salt, IPROTO_SALT_SIZE);
> +	assert(evio_has_fd(&con->output));
> +	ev_feed_event(con->loop, &con->output, EV_WRITE);
> +}

Re: [Tarantool-patches] [PATCH v6 3/3] iproto: move greeting from tx thread to iproto

[Ilya Kosarev]

[-- Attachment #1: Type: text/plain, Size: 6129 bytes --]


Hi!
 
Thanks for your review.
 
Sent v7 considering your comments. Some answers below. 
>Среда, 16 декабря 2020, 2:04 +03:00 от Vladislav Shpilevoy <v.shpilevoy@tarantool.org>:
> 
>Hi! Thanks for the patch!
>
>See 4 comments below.
>
>On 13.12.2020 00:38, Ilya Kosarev wrote:
>> On connection, an evio service callback is invoked to accept it. The
>> next step after acceptance was to process connection to tx thread
>> through cbus. This meant that any connection interaction involves tx
>> thread even before we get to send the greeting to the client.
>> Consequently, the client might reach the instance with enormous number
>> of connections, leading to the file descriptors limit exhaustion in
>> case of unresponsive tx thread (for example, when building secondary
>> index) and extra tx_process_connects afterwords, even in case the
>> instance doesn't fail with too many open files error.
>> This patch allows iproto to accept connection and send greeting by
>> itself. Thus the connection is being established and stays in
>> fetch_schema state while tx thread is unresponsive.
>>
>> Closes #3776
>> ---
>> src/box/iproto.cc | 118 +++++++++++++-----
>> test/app/gh-4787-netbox-empty-errmsg.result | 18 ---
>> test/app/gh-4787-netbox-empty-errmsg.test.lua | 8 --
>> 3 files changed, 88 insertions(+), 56 deletions(-)
>>
>> diff --git a/src/box/iproto.cc b/src/box/iproto.cc
>> index f7330af21d..a7da115925 100644
>> --- a/src/box/iproto.cc
>> +++ b/src/box/iproto.cc
>> @@ -1091,6 +1093,46 @@ iproto_connection_on_output(ev_loop *loop, struct ev_io *watcher,
>> }
>> }
>>
>> +static int
>> +iproto_buf_flush(struct iproto_connection *con)
>
>1. The function lacks a comment. It is not so trivial why do we
>have 2 flush functions. Also maybe worth renaming it to
>iproto_connection_greeting_flush. Because it is a method of
>'iproto_connection' (thus the prefix), and works with the
>greeting only.
Right, fixed in v7 of the patch.
>
>> +{
>> + struct iovec greeting;
>> + greeting.iov_base = &con->greeting_buf;
>> + greeting.iov_len = IPROTO_GREETING_SIZE;
>> + ssize_t nwr = sio_writev(con->output.fd, &greeting, 1);
>> +
>> + if (nwr > 0) {
>> + /* Count statistics */
>> + rmean_collect(rmean_net, IPROTO_SENT, nwr);
>> + return 1;
>> + } else if (nwr < 0 && !sio_wouldblock(errno)) {
>> + return -1;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static void
>> +iproto_connection_on_greeting(ev_loop *loop, struct ev_io *watcher,
>> + int /* revents */)
>> +{
>> + struct iproto_connection *con =
>> + (struct iproto_connection *)watcher->data;
>> + int rc = iproto_buf_flush(con);
>> + if (rc <= 0) {
>> + if (rc == 0) {
>> + ev_io_start(loop, &con->output);
>> + } else {
>> + diag_log();
>> + con->state = IPROTO_CONNECTION_CLOSED;
>
>2. I don't understand how this part works. You didn't close the
>socket, and didn't delete it from libev. But IPROTO_CONNECTION_CLOSED
>clearly states
>
>/**
>* Socket was closed, a notification is sent to the TX
>* thread to close the session.
>*/
>
>The way you change it here does not match the description. AFAIU,
>the only legal function able to set this state is
>iproto_connection_close(), and it should remain so.
>
>Moreover, I see this should crash in an assertion in
>iproto_connection_close(). Because this function will be called by
>net_finish_connect() due to state being CLOSED (which is weird -
>why is close() called if the state is already CLOSED?). But inside
>it has "assert(con->state == IPROTO_CONNECTION_ALIVE)" in case the
>ev_watcher still has a descriptor - it still has, so it will go there
>any crash.
>
>Did I miss something?
Yes, my bad. It definitely needs new state. There was not going to be
an assertion fail as long as i switch it back, but it is still not good. Fixed in v7.
>
>> + }
>> + return;
>> + }
>> + ev_io_stop(con->loop, &con->output);
>> + ev_io_init(&con->output, iproto_connection_on_output,
>> + con->output.fd, EV_WRITE);
>> +}
>
>3. What if sio_writev() has written only half of the bytes?
>You didn't check and immediately proceeded to the normal
>operation mode. But the greeting wasn't sent yet. Not all.
>
>Also you don't propagate iov_base in case only a part of
>the data was sent.
>
>You can easily check it using a temporary error injection.
>Before sio_writev() try to inject iov_len =
>max(iov_len, IPROTO_GREETING_SIZE / 2) to force it to work
>in 2 iterations, and you will see the issue.
>
>Partial write is a rare thing, but it is totally real. It
>is not some kind of an impossible thing only from the books.
>Even if it is just 128 bytes.
Fixed in v7.
>
>> +
>> static struct iproto_connection *
>> iproto_connection_new(int fd)
>> {
>> @@ -1938,50 +1980,64 @@ net_end_subscribe(struct cmsg *m)
>> }
>>
>> /**
>> - * Handshake a connection: invoke the on-connect trigger
>> - * and possibly authenticate. Try to send the client an error
>> - * upon a failure.
>> + * Handshake a connection: send greeting for it.
>> + */
>> +static void
>> +iproto_process_connect(struct iproto_msg *msg)
>
>4. Why do you take iproto_msg object if the only thing you do
>with it is take the connection object? Wouldn't it be simpler
>and more understandable, if it would be iproto_connection_start()
>or something like that? With an iproto_connection * argument.
Fixed in v7.
>
>> +{
>> + struct iproto_connection *con = msg->connection;
>> + /*
>> + * INSTANCE_UUID is guaranteed to be inited before this moment.
>> + * We start listening either in local_recovery() or bootstrap().
>> + * The INSTANCE_UUID is ensured to be inited in the beginning of
>> + * both methods. In case of local_recovery() it is verified that
>> + * INSTANCE_UUID was read from the snapshot in memtx_engine_new().
>> + * In bootstrap() INSTANCE_UUID is either taken from the
>> + * instance_uuid box.cfg{} param or created on the spot.
>> + */
>> + random_bytes(con->salt, IPROTO_SALT_SIZE);
>> + greeting_encode(con->greeting_buf, tarantool_version_id(),
>> + &INSTANCE_UUID, con->salt, IPROTO_SALT_SIZE);
>> + assert(evio_has_fd(&con->output));
>> + ev_feed_event(con->loop, &con->output, EV_WRITE);
>> +} 
 
 
--
Ilya Kosarev
 

[-- Attachment #2: Type: text/html, Size: 8059 bytes --]