* [PATCH] iproto: init coio watcher before join/subscribe
@ 2019-05-15 20:57 Alexander Turenko
2019-05-16 8:38 ` [tarantool-patches] " Konstantin Osipov
0 siblings, 1 reply; 5+ messages in thread
From: Alexander Turenko @ 2019-05-15 20:57 UTC (permalink / raw)
To: Vladimir Davydov; +Cc: Alexander Turenko, tarantool-patches
box_process_join() and box_process_subscribe() use coio_write_xrow(),
which calls coio_writev_timeout() under hood. If a socket will block at
write() the function calls ev_io_start() to wake the fiber up when the
socket will be ready to write. This code assumes that the watcher
(struct ev_io) is initialized as coio watcher, i.e. coio_create() has
been called.
The reason why the code works before is that coio_write_xrow() in
box_process_{join,subscribe}() writes a small piece of data and so the
situation when a socket write buffer has less free space then needed is
rare.
Fixes #4110.
---
How to reproduce the issue:
| echo 'use_unix_sockets_iproto = True' >> test/vinyl/suite.ini
| (cd test && ./test-run.py vinyl/replica_rejoin.test.lua)
It is hard to write a test w/o a network emulation layer: we should
trigger a situation when a socket write buffer is almost full right
after initial join data sending (but before vclock sending).
I don't sure I use terminology in the right way in the commit message.
Please, let me know if the wording feels bad.
https://github.com/tarantool/tarantool/issues/4110
https://github.com/tarantool/tarantool/tree/Totktonada/gh-4110-fix-segfault-in-iproto
src/box/iproto.cc | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/src/box/iproto.cc b/src/box/iproto.cc
index 02b558ede..8f899fed8 100644
--- a/src/box/iproto.cc
+++ b/src/box/iproto.cc
@@ -1693,6 +1693,8 @@ tx_process_join_subscribe(struct cmsg *m)
{
struct iproto_msg *msg = tx_accept_msg(m);
struct iproto_connection *con = msg->connection;
+ struct ev_io io;
+ coio_create(&io, con->input.fd);
try {
switch (msg->header.type) {
case IPROTO_JOIN:
@@ -1701,7 +1703,7 @@ tx_process_join_subscribe(struct cmsg *m)
* the lambda in the beginning of the block
* will re-activate the watchers for us.
*/
- box_process_join(&con->input, &msg->header);
+ box_process_join(&io, &msg->header);
break;
case IPROTO_SUBSCRIBE:
/*
@@ -1710,7 +1712,7 @@ tx_process_join_subscribe(struct cmsg *m)
* the write watcher will be re-activated
* the same way as for JOIN.
*/
- box_process_subscribe(&con->input, &msg->header);
+ box_process_subscribe(&io, &msg->header);
break;
default:
unreachable();
--
2.21.0
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [tarantool-patches] [PATCH] iproto: init coio watcher before join/subscribe
2019-05-15 20:57 [PATCH] iproto: init coio watcher before join/subscribe Alexander Turenko
@ 2019-05-16 8:38 ` Konstantin Osipov
2019-05-16 11:46 ` Alexander Turenko
0 siblings, 1 reply; 5+ messages in thread
From: Konstantin Osipov @ 2019-05-16 8:38 UTC (permalink / raw)
To: tarantool-patches; +Cc: Vladimir Davydov, Alexander Turenko
* Alexander Turenko <alexander.turenko@tarantool.org> [19/05/16 09:53]:
> box_process_join() and box_process_subscribe() use coio_write_xrow(),
> which calls coio_writev_timeout() under hood. If a socket will block at
> write() the function calls ev_io_start() to wake the fiber up when the
> socket will be ready to write. This code assumes that the watcher
> (struct ev_io) is initialized as coio watcher, i.e. coio_create() has
> been called.
the same comment is missing from the source code.
--
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [tarantool-patches] [PATCH] iproto: init coio watcher before join/subscribe
2019-05-16 8:38 ` [tarantool-patches] " Konstantin Osipov
@ 2019-05-16 11:46 ` Alexander Turenko
2019-05-20 9:41 ` Vladimir Davydov
0 siblings, 1 reply; 5+ messages in thread
From: Alexander Turenko @ 2019-05-16 11:46 UTC (permalink / raw)
To: Konstantin Osipov; +Cc: tarantool-patches, Vladimir Davydov
On Thu, May 16, 2019 at 11:38:17AM +0300, Konstantin Osipov wrote:
> * Alexander Turenko <alexander.turenko@tarantool.org> [19/05/16 09:53]:
> > box_process_join() and box_process_subscribe() use coio_write_xrow(),
> > which calls coio_writev_timeout() under hood. If a socket will block at
> > write() the function calls ev_io_start() to wake the fiber up when the
> > socket will be ready to write. This code assumes that the watcher
> > (struct ev_io) is initialized as coio watcher, i.e. coio_create() has
> > been called.
>
> the same comment is missing from the source code.
Looks like it is the contract of box_process_* functions, so I added comments
to them. After that the change in iproto.cc is obvious and I think don't
requires a comment.
See the full patch below.
diff --git a/src/box/box.h b/src/box/box.h
index 53d88ab71..ddcfbe2e5 100644
--- a/src/box/box.h
+++ b/src/box/box.h
@@ -172,9 +172,25 @@ box_reset_stat(void);
void
box_process_auth(struct auth_request *request, const char *salt);
+/**
+ * Join a replica.
+ *
+ * Register a replica and feed it with data.
+ *
+ * \param io coio watcher (initialized with coio_create())
+ * \param JOIN packet header
+ */
void
box_process_join(struct ev_io *io, struct xrow_header *header);
+/**
+ * Subscribe a replica.
+ *
+ * Perform necessary checks and start a relay thread.
+ *
+ * \param io coio watcher (initialized with coio_create())
+ * \param SUBSCRIBE packet header
+ */
void
box_process_subscribe(struct ev_io *io, struct xrow_header *header);
diff --git a/src/box/iproto.cc b/src/box/iproto.cc
index 02b558ede..8f899fed8 100644
--- a/src/box/iproto.cc
+++ b/src/box/iproto.cc
@@ -1693,6 +1693,8 @@ tx_process_join_subscribe(struct cmsg *m)
{
struct iproto_msg *msg = tx_accept_msg(m);
struct iproto_connection *con = msg->connection;
+ struct ev_io io;
+ coio_create(&io, con->input.fd);
try {
switch (msg->header.type) {
case IPROTO_JOIN:
@@ -1701,7 +1703,7 @@ tx_process_join_subscribe(struct cmsg *m)
* the lambda in the beginning of the block
* will re-activate the watchers for us.
*/
- box_process_join(&con->input, &msg->header);
+ box_process_join(&io, &msg->header);
break;
case IPROTO_SUBSCRIBE:
/*
@@ -1710,7 +1712,7 @@ tx_process_join_subscribe(struct cmsg *m)
* the write watcher will be re-activated
* the same way as for JOIN.
*/
- box_process_subscribe(&con->input, &msg->header);
+ box_process_subscribe(&io, &msg->header);
break;
default:
unreachable();
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [tarantool-patches] [PATCH] iproto: init coio watcher before join/subscribe
2019-05-16 11:46 ` Alexander Turenko
@ 2019-05-20 9:41 ` Vladimir Davydov
2019-05-20 9:52 ` Vladimir Davydov
0 siblings, 1 reply; 5+ messages in thread
From: Vladimir Davydov @ 2019-05-20 9:41 UTC (permalink / raw)
To: Alexander Turenko; +Cc: Konstantin Osipov, tarantool-patches
Pushed to master and 2.1.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [tarantool-patches] [PATCH] iproto: init coio watcher before join/subscribe
2019-05-20 9:41 ` Vladimir Davydov
@ 2019-05-20 9:52 ` Vladimir Davydov
0 siblings, 0 replies; 5+ messages in thread
From: Vladimir Davydov @ 2019-05-20 9:52 UTC (permalink / raw)
To: Alexander Turenko; +Cc: Konstantin Osipov, tarantool-patches
On Mon, May 20, 2019 at 12:41:38PM +0300, Vladimir Davydov wrote:
> Pushed to master and 2.1.
Pushed to 1.10 as well as this fixes a crash, let alone rare one.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-05-20 9:52 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-15 20:57 [PATCH] iproto: init coio watcher before join/subscribe Alexander Turenko
2019-05-16 8:38 ` [tarantool-patches] " Konstantin Osipov
2019-05-16 11:46 ` Alexander Turenko
2019-05-20 9:41 ` Vladimir Davydov
2019-05-20 9:52 ` Vladimir Davydov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox