* [PATCH 1/1] iproto: get iproto obuf only right before usage
[not found] <20180316135103.usjz6veqe5mpste6@esperanza>
@ 2018-03-16 14:41 ` Vladislav Shpilevoy
2018-03-20 13:07 ` Vladimir Davydov
0 siblings, 1 reply; 4+ messages in thread
From: Vladislav Shpilevoy @ 2018-03-16 14:41 UTC (permalink / raw)
To: tarantool-patches; +Cc: vdavydov.dev, Vladislav Shpilevoy
--- Branch: https://github.com/tarantool/tarantool/tree/fix-iproto-buffers
--- There is no issue, since the fix is simple, and does not
--- affect external behavior.
--- Changes: do not rely on buffers number and rotation. Just
--- send multiple fast read/write requests with a small delay.
--- With no patch the test hangs or crashes since some responses
--- are corrupted or discarded.
It is possible to discard non-sent responses using a special
sequence of requests and yields. In details: if DML requests
yield on commit too long, there are fast read requests, and a
network is saturated, then some non-sent DML responses are
discarded.
Signed-off-by: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
---
src/box/iproto.cc | 22 ++++++++++++++++++----
src/errinj.h | 1 +
test/box/errinj.result | 42 +++++++++++++++++++++++++++++++++++++++++-
test/box/errinj.test.lua | 16 ++++++++++++++++
4 files changed, 76 insertions(+), 5 deletions(-)
diff --git a/src/box/iproto.cc b/src/box/iproto.cc
index 37313fa52..41c60c318 100644
--- a/src/box/iproto.cc
+++ b/src/box/iproto.cc
@@ -59,6 +59,7 @@
#include "replication.h" /* instance_uuid */
#include "iproto_constants.h"
#include "rmean.h"
+#include "errinj.h"
/* The number of iproto messages in flight */
enum { IPROTO_MSG_MAX = 768 };
@@ -1167,7 +1168,6 @@ static void
tx_process1(struct cmsg *m)
{
struct iproto_msg *msg = tx_accept_msg(m);
- struct obuf *out = msg->connection->tx.p_obuf;
tx_fiber_init(msg->connection->session, msg->header.sync);
if (tx_check_schema(msg->header.schema_version))
@@ -1175,8 +1175,15 @@ tx_process1(struct cmsg *m)
struct tuple *tuple;
struct obuf_svp svp;
- if (box_process1(&msg->dml, &tuple) ||
- iproto_prepare_select(out, &svp))
+ ERROR_INJECT(ERRINJ_IPROTO_TX_DELAY, {
+ if (rand() % 100 < 10)
+ fiber_sleep(0.001);
+ });
+ struct obuf *out;
+ if (box_process1(&msg->dml, &tuple) != 0)
+ goto error;
+ out = msg->connection->tx.p_obuf;
+ if (iproto_prepare_select(out, &svp) != 0)
goto error;
if (tuple && tuple_to_obuf(tuple, out))
goto error;
@@ -1192,7 +1199,7 @@ static void
tx_process_select(struct cmsg *m)
{
struct iproto_msg *msg = tx_accept_msg(m);
- struct obuf *out = msg->connection->tx.p_obuf;
+ struct obuf *out;
struct obuf_svp svp;
struct port port;
int count;
@@ -1204,11 +1211,18 @@ tx_process_select(struct cmsg *m)
if (tx_check_schema(msg->header.schema_version))
goto error;
+ ERROR_INJECT(ERRINJ_IPROTO_TX_DELAY, {
+ /* Select can yield, for example, on vinyl. */
+ if (rand() % 100 < 10)
+ fiber_sleep(0.001);
+ });
rc = box_select(req->space_id, req->index_id,
req->iterator, req->offset, req->limit,
req->key, req->key_end, &port);
if (rc < 0)
goto error;
+
+ out = msg->connection->tx.p_obuf;
if (iproto_prepare_select(out, &svp) != 0) {
port_destroy(&port);
goto error;
diff --git a/src/errinj.h b/src/errinj.h
index 352a3c3c9..a5417baf6 100644
--- a/src/errinj.h
+++ b/src/errinj.h
@@ -107,6 +107,7 @@ struct errinj {
_(ERRINJ_RELAY_EXIT_DELAY, ERRINJ_DOUBLE, {.dparam = 0}) \
_(ERRINJ_VY_DELAY_PK_LOOKUP, ERRINJ_BOOL, {.bparam = false}) \
_(ERRINJ_VY_RUN_WRITE_STMT_TIMEOUT, ERRINJ_DOUBLE, {.dparam = 0}) \
+ _(ERRINJ_IPROTO_TX_DELAY, ERRINJ_BOOL, {.bparam = false}) \
ENUM0(errinj_id, ERRINJ_LIST);
extern struct errinj errinjs[];
diff --git a/test/box/errinj.result b/test/box/errinj.result
index db27dd4b3..3ccf196f8 100644
--- a/test/box/errinj.result
+++ b/test/box/errinj.result
@@ -60,9 +60,11 @@ errinj.info()
state: false
ERRINJ_WAL_ROTATE:
state: false
+ ERRINJ_VY_POINT_ITER_WAIT:
+ state: false
ERRINJ_RELAY_EXIT_DELAY:
state: 0
- ERRINJ_VY_POINT_ITER_WAIT:
+ ERRINJ_IPROTO_TX_DELAY:
state: false
ERRINJ_TUPLE_FIELD:
state: false
@@ -1057,3 +1059,41 @@ box.error.injection.set('ERRINJ_BUILD_SECONDARY', -1)
s:drop()
---
...
+box.schema.user.grant('guest', 'read,write,execute', 'universe')
+---
+...
+s = box.schema.space.create('test')
+---
+...
+_ = s:create_index('pk')
+---
+...
+c = net_box.connect(box.cfg.listen)
+---
+...
+ch = fiber.channel(200)
+---
+...
+errinj.set("ERRINJ_IPROTO_TX_DELAY", true)
+---
+- ok
+...
+for i = 1, 100 do fiber.create(function() for j = 1, 10 do c.space.test:replace{1} end ch:put(true) end) end
+---
+...
+for i = 1, 100 do fiber.create(function() for j = 1, 10 do c.space.test:select() end ch:put(true) end) end
+---
+...
+for i = 1, 200 do ch:get() end
+---
+...
+errinj.set("ERRINJ_IPROTO_TX_DELAY", false)
+---
+- ok
+...
+s:drop()
+---
+...
+box.schema.user.revoke('guest', 'read,write,execute','universe')
+---
+...
diff --git a/test/box/errinj.test.lua b/test/box/errinj.test.lua
index 9bcf53847..a24b357e6 100644
--- a/test/box/errinj.test.lua
+++ b/test/box/errinj.test.lua
@@ -348,3 +348,19 @@ box.error.injection.set('ERRINJ_BUILD_SECONDARY', sk.id)
sk:alter({parts = {2, 'number'}})
box.error.injection.set('ERRINJ_BUILD_SECONDARY', -1)
s:drop()
+
+box.schema.user.grant('guest', 'read,write,execute', 'universe')
+s = box.schema.space.create('test')
+_ = s:create_index('pk')
+
+c = net_box.connect(box.cfg.listen)
+
+ch = fiber.channel(200)
+errinj.set("ERRINJ_IPROTO_TX_DELAY", true)
+for i = 1, 100 do fiber.create(function() for j = 1, 10 do c.space.test:replace{1} end ch:put(true) end) end
+for i = 1, 100 do fiber.create(function() for j = 1, 10 do c.space.test:select() end ch:put(true) end) end
+for i = 1, 200 do ch:get() end
+errinj.set("ERRINJ_IPROTO_TX_DELAY", false)
+
+s:drop()
+box.schema.user.revoke('guest', 'read,write,execute','universe')
--
2.14.3 (Apple Git-98)
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 1/1] iproto: get iproto obuf only right before usage
2018-03-16 14:41 ` [PATCH 1/1] iproto: get iproto obuf only right before usage Vladislav Shpilevoy
@ 2018-03-20 13:07 ` Vladimir Davydov
2018-03-20 13:43 ` [PATCH v3 " Vladislav Shpilevoy
0 siblings, 1 reply; 4+ messages in thread
From: Vladimir Davydov @ 2018-03-20 13:07 UTC (permalink / raw)
To: Vladislav Shpilevoy; +Cc: tarantool-patches
On Fri, Mar 16, 2018 at 05:41:42PM +0300, Vladislav Shpilevoy wrote:
> --- Branch: https://github.com/tarantool/tarantool/tree/fix-iproto-buffers
> --- There is no issue, since the fix is simple, and does not
> --- affect external behavior.
Please open a Github ticket. Just describe the problem there. A test
case is not necessary. This will be useful for release changelog.
(FYI I asked both Kostja and Kirill - they both agree).
>
> --- Changes: do not rely on buffers number and rotation. Just
> --- send multiple fast read/write requests with a small delay.
> --- With no patch the test hangs or crashes since some responses
> --- are corrupted or discarded.
When I talked about extra info you don't want to be included into the
commit message, I meant the '---' generated by git-format-patch, as
everything following '---' is ignored by git-am.
Also, please don't forget to add the version to the subject line
(--subject-prefix='PATCH v2').
>
> It is possible to discard non-sent responses using a special
> sequence of requests and yields. In details: if DML requests
> yield on commit too long, there are fast read requests, and a
> network is saturated, then some non-sent DML responses are
> discarded.
>
> Signed-off-by: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
We don't sign patches, not yet. Please fix your git config.
> @@ -1175,8 +1175,15 @@ tx_process1(struct cmsg *m)
>
> struct tuple *tuple;
> struct obuf_svp svp;
> - if (box_process1(&msg->dml, &tuple) ||
> - iproto_prepare_select(out, &svp))
> + ERROR_INJECT(ERRINJ_IPROTO_TX_DELAY, {
> + if (rand() % 100 < 10)
> + fiber_sleep(0.001);
> + });
> + struct obuf *out;
> + if (box_process1(&msg->dml, &tuple) != 0)
> + goto error;
> + out = msg->connection->tx.p_obuf;
> + if (iproto_prepare_select(out, &svp) != 0)
> goto error;
> if (tuple && tuple_to_obuf(tuple, out))
> goto error;
> @@ -1192,7 +1199,7 @@ static void
> tx_process_select(struct cmsg *m)
> {
> struct iproto_msg *msg = tx_accept_msg(m);
> - struct obuf *out = msg->connection->tx.p_obuf;
> + struct obuf *out;
> struct obuf_svp svp;
> struct port port;
> int count;
> @@ -1204,11 +1211,18 @@ tx_process_select(struct cmsg *m)
> if (tx_check_schema(msg->header.schema_version))
> goto error;
>
> + ERROR_INJECT(ERRINJ_IPROTO_TX_DELAY, {
> + /* Select can yield, for example, on vinyl. */
> + if (rand() % 100 < 10)
> + fiber_sleep(0.001);
> + });
There are quite a few constants in this error injection so
copying-and-pasting doesn't look good. I'd wrap it into an inline
function (tx_inject_delay or something, think of the name pls).
> diff --git a/test/box/errinj.test.lua b/test/box/errinj.test.lua
> index 9bcf53847..a24b357e6 100644
> --- a/test/box/errinj.test.lua
> +++ b/test/box/errinj.test.lua
> @@ -348,3 +348,19 @@ box.error.injection.set('ERRINJ_BUILD_SECONDARY', sk.id)
> sk:alter({parts = {2, 'number'}})
> box.error.injection.set('ERRINJ_BUILD_SECONDARY', -1)
> s:drop()
Please add a brief comment explaining what and why we are testing here.
> +
> +box.schema.user.grant('guest', 'read,write,execute', 'universe')
> +s = box.schema.space.create('test')
> +_ = s:create_index('pk')
> +
> +c = net_box.connect(box.cfg.listen)
> +
> +ch = fiber.channel(200)
> +errinj.set("ERRINJ_IPROTO_TX_DELAY", true)
> +for i = 1, 100 do fiber.create(function() for j = 1, 10 do c.space.test:replace{1} end ch:put(true) end) end
> +for i = 1, 100 do fiber.create(function() for j = 1, 10 do c.space.test:select() end ch:put(true) end) end
> +for i = 1, 200 do ch:get() end
> +errinj.set("ERRINJ_IPROTO_TX_DELAY", false)
> +
> +s:drop()
> +box.schema.user.revoke('guest', 'read,write,execute','universe')
^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH v3 1/1] iproto: get iproto obuf only right before usage
2018-03-20 13:07 ` Vladimir Davydov
@ 2018-03-20 13:43 ` Vladislav Shpilevoy
2018-03-20 16:49 ` Vladimir Davydov
0 siblings, 1 reply; 4+ messages in thread
From: Vladislav Shpilevoy @ 2018-03-20 13:43 UTC (permalink / raw)
To: tarantool-patches; +Cc: vdavydov.dev, Vladislav Shpilevoy
It is possible to discard non-sent responses using a special
sequence of requests and yields. In details: if DML requests
yield on commit too long, there are fast read requests, and a
network is saturated, then some non-sent DML responses are
discarded.
Closes #3255
---
src/box/iproto.cc | 25 +++++++++++++++++++++----
src/errinj.h | 1 +
test/box/errinj.result | 46 +++++++++++++++++++++++++++++++++++++++++++++-
test/box/errinj.test.lua | 21 +++++++++++++++++++++
4 files changed, 88 insertions(+), 5 deletions(-)
diff --git a/src/box/iproto.cc b/src/box/iproto.cc
index 37313fa52..db5820806 100644
--- a/src/box/iproto.cc
+++ b/src/box/iproto.cc
@@ -59,6 +59,7 @@
#include "replication.h" /* instance_uuid */
#include "iproto_constants.h"
#include "rmean.h"
+#include "errinj.h"
/* The number of iproto messages in flight */
enum { IPROTO_MSG_MAX = 768 };
@@ -1163,11 +1164,20 @@ tx_reply_iproto_error(struct cmsg *m)
iproto_wpos_create(&msg->wpos, out);
}
+/** Inject a short delay on tx request processing for testing. */
+static inline void
+tx_inject_delay()
+{
+ ERROR_INJECT(ERRINJ_IPROTO_TX_DELAY, {
+ if (rand() % 100 < 10)
+ fiber_sleep(0.001);
+ });
+}
+
static void
tx_process1(struct cmsg *m)
{
struct iproto_msg *msg = tx_accept_msg(m);
- struct obuf *out = msg->connection->tx.p_obuf;
tx_fiber_init(msg->connection->session, msg->header.sync);
if (tx_check_schema(msg->header.schema_version))
@@ -1175,8 +1185,12 @@ tx_process1(struct cmsg *m)
struct tuple *tuple;
struct obuf_svp svp;
- if (box_process1(&msg->dml, &tuple) ||
- iproto_prepare_select(out, &svp))
+ struct obuf *out;
+ tx_inject_delay();
+ if (box_process1(&msg->dml, &tuple) != 0)
+ goto error;
+ out = msg->connection->tx.p_obuf;
+ if (iproto_prepare_select(out, &svp) != 0)
goto error;
if (tuple && tuple_to_obuf(tuple, out))
goto error;
@@ -1192,7 +1206,7 @@ static void
tx_process_select(struct cmsg *m)
{
struct iproto_msg *msg = tx_accept_msg(m);
- struct obuf *out = msg->connection->tx.p_obuf;
+ struct obuf *out;
struct obuf_svp svp;
struct port port;
int count;
@@ -1204,11 +1218,14 @@ tx_process_select(struct cmsg *m)
if (tx_check_schema(msg->header.schema_version))
goto error;
+ tx_inject_delay();
rc = box_select(req->space_id, req->index_id,
req->iterator, req->offset, req->limit,
req->key, req->key_end, &port);
if (rc < 0)
goto error;
+
+ out = msg->connection->tx.p_obuf;
if (iproto_prepare_select(out, &svp) != 0) {
port_destroy(&port);
goto error;
diff --git a/src/errinj.h b/src/errinj.h
index 352a3c3c9..a5417baf6 100644
--- a/src/errinj.h
+++ b/src/errinj.h
@@ -107,6 +107,7 @@ struct errinj {
_(ERRINJ_RELAY_EXIT_DELAY, ERRINJ_DOUBLE, {.dparam = 0}) \
_(ERRINJ_VY_DELAY_PK_LOOKUP, ERRINJ_BOOL, {.bparam = false}) \
_(ERRINJ_VY_RUN_WRITE_STMT_TIMEOUT, ERRINJ_DOUBLE, {.dparam = 0}) \
+ _(ERRINJ_IPROTO_TX_DELAY, ERRINJ_BOOL, {.bparam = false}) \
ENUM0(errinj_id, ERRINJ_LIST);
extern struct errinj errinjs[];
diff --git a/test/box/errinj.result b/test/box/errinj.result
index db27dd4b3..cd0c35c2f 100644
--- a/test/box/errinj.result
+++ b/test/box/errinj.result
@@ -60,9 +60,11 @@ errinj.info()
state: false
ERRINJ_WAL_ROTATE:
state: false
+ ERRINJ_VY_POINT_ITER_WAIT:
+ state: false
ERRINJ_RELAY_EXIT_DELAY:
state: 0
- ERRINJ_VY_POINT_ITER_WAIT:
+ ERRINJ_IPROTO_TX_DELAY:
state: false
ERRINJ_TUPLE_FIELD:
state: false
@@ -1057,3 +1059,45 @@ box.error.injection.set('ERRINJ_BUILD_SECONDARY', -1)
s:drop()
---
...
+--
+-- gh-3255: iproto can crash and discard responses, if a network
+-- is saturated, and DML yields too long on commit.
+--
+box.schema.user.grant('guest', 'read,write,execute', 'universe')
+---
+...
+s = box.schema.space.create('test')
+---
+...
+_ = s:create_index('pk')
+---
+...
+c = net_box.connect(box.cfg.listen)
+---
+...
+ch = fiber.channel(200)
+---
+...
+errinj.set("ERRINJ_IPROTO_TX_DELAY", true)
+---
+- ok
+...
+for i = 1, 100 do fiber.create(function() for j = 1, 10 do c.space.test:replace{1} end ch:put(true) end) end
+---
+...
+for i = 1, 100 do fiber.create(function() for j = 1, 10 do c.space.test:select() end ch:put(true) end) end
+---
+...
+for i = 1, 200 do ch:get() end
+---
+...
+errinj.set("ERRINJ_IPROTO_TX_DELAY", false)
+---
+- ok
+...
+s:drop()
+---
+...
+box.schema.user.revoke('guest', 'read,write,execute','universe')
+---
+...
diff --git a/test/box/errinj.test.lua b/test/box/errinj.test.lua
index 9bcf53847..3af1b74dc 100644
--- a/test/box/errinj.test.lua
+++ b/test/box/errinj.test.lua
@@ -348,3 +348,24 @@ box.error.injection.set('ERRINJ_BUILD_SECONDARY', sk.id)
sk:alter({parts = {2, 'number'}})
box.error.injection.set('ERRINJ_BUILD_SECONDARY', -1)
s:drop()
+
+--
+-- gh-3255: iproto can crash and discard responses, if a network
+-- is saturated, and DML yields too long on commit.
+--
+
+box.schema.user.grant('guest', 'read,write,execute', 'universe')
+s = box.schema.space.create('test')
+_ = s:create_index('pk')
+
+c = net_box.connect(box.cfg.listen)
+
+ch = fiber.channel(200)
+errinj.set("ERRINJ_IPROTO_TX_DELAY", true)
+for i = 1, 100 do fiber.create(function() for j = 1, 10 do c.space.test:replace{1} end ch:put(true) end) end
+for i = 1, 100 do fiber.create(function() for j = 1, 10 do c.space.test:select() end ch:put(true) end) end
+for i = 1, 200 do ch:get() end
+errinj.set("ERRINJ_IPROTO_TX_DELAY", false)
+
+s:drop()
+box.schema.user.revoke('guest', 'read,write,execute','universe')
--
2.14.3 (Apple Git-98)
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v3 1/1] iproto: get iproto obuf only right before usage
2018-03-20 13:43 ` [PATCH v3 " Vladislav Shpilevoy
@ 2018-03-20 16:49 ` Vladimir Davydov
0 siblings, 0 replies; 4+ messages in thread
From: Vladimir Davydov @ 2018-03-20 16:49 UTC (permalink / raw)
To: Vladislav Shpilevoy; +Cc: tarantool-patches
On Tue, Mar 20, 2018 at 04:43:05PM +0300, Vladislav Shpilevoy wrote:
> It is possible to discard non-sent responses using a special
> sequence of requests and yields. In details: if DML requests
> yield on commit too long, there are fast read requests, and a
> network is saturated, then some non-sent DML responses are
> discarded.
>
> Closes #3255
> ---
> src/box/iproto.cc | 25 +++++++++++++++++++++----
> src/errinj.h | 1 +
> test/box/errinj.result | 46 +++++++++++++++++++++++++++++++++++++++++++++-
> test/box/errinj.test.lua | 21 +++++++++++++++++++++
> 4 files changed, 88 insertions(+), 5 deletions(-)
Looks good to me.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2018-03-20 16:49 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <20180316135103.usjz6veqe5mpste6@esperanza>
2018-03-16 14:41 ` [PATCH 1/1] iproto: get iproto obuf only right before usage Vladislav Shpilevoy
2018-03-20 13:07 ` Vladimir Davydov
2018-03-20 13:43 ` [PATCH v3 " Vladislav Shpilevoy
2018-03-20 16:49 ` Vladimir Davydov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox