<html><head><meta http-equiv="Content-Type" content="text/html; charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; line-break: after-white-space;" class=""><div class=""><br class="Apple-interchange-newline">
</div>
<div><br class=""><blockquote type="cite" class=""><div class="">28 февр. 2020 г., в 02:22, Vladislav Shpilevoy <<a href="mailto:v.shpilevoy@tarantool.org" class="">v.shpilevoy@tarantool.org</a>> написал(а):</div><br class="Apple-interchange-newline"><div class=""><div class=""><blockquote type="cite" class="">Ok. I tried to add either ERROR_INJECT_YIELD or ERROR_INJECT_SLEEP at the end<br class="">of wal_write_to_disk().<br class="">It looks like you cannot yield in wal_write_to_disk(). (is it possible to yield in WAL thread at all?)<br class="">Firing this injection with ERROR_INJECT_YIELD and then resetting it leads to wal thread stopping<br class="">processing messages. This leads to tarantool hanging infinitely on shutdown when tx waits for wal<br class="">thread to exit, but wal never gets the shutdown signal.<br class=""> <br class="">Using ERROR_INJECT_SLEEP leads to wal watchers not being notified until the injection is reset. This<br class="">probably happens because of wal_notify_watchers’ use of cpipe_flush_input(), which doesn’t flush the input until<br class="">the end of event loop iteration, if there are not enough messages (only one message in our case).<br class="">The event loop iteration never ends, because we sleep right after wal_notify_watchers() call.<br class=""> <br class="">So, I see skipping vclock assignment in tx_schedule_commit() as the only possible alternative.<br class="">Hope my explanation was clear enough and, more imortantly, correct. If not, lest discuss.<br class=""></blockquote><br class="">Yeah, I stumbled into the same problems. And realized that the current test,<br class="">after all, is not valid. So we need to change it anyway.<br class=""><br class="">First I tried to solve them by trying to block TX thread totally on one<br class="">instance after it tried to commit something. Since that would block the<br class="">test too, I tried to introduce a new thread - errinj thread, which would<br class="">listen on an ip/port or a unix socket, and will receive requests to set<br class="">error injections from another instance. So rebootstrap1's TX thread would<br class="">freeze, and I could control that instance via interaction with its errinj<br class="">thread from rebootstrap2 instance or from default instance.<br class=""><br class="">Despite the idea would work for sure, it appeared to be hard to implement<br class="">for a short time, so I postponed that, and probably will open a ticket to<br class="">implement such thing. It could be useful for any test, which needs to test<br class="">behaviour of other threads, when TX is not scheduled for a long time. Also<br class="">we can implement that logic as a part of iproto thread.<br class=""><br class="">For our case I found a simpler solution - sleep in wal_write_to_disk, but<br class="">deliver all watcher events immediately. Then the test works. And still<br class="">crashes without your patch.<br class=""></div></div></blockquote><div><br class=""></div><div>Thanks for your amendments!</div><div>The new test LGTM. Do you mean the old test is incorrect because the</div><div>injection simulates an impossible situation? If yes, then I agree.</div><div>I applied your diff with a tiny fix. See below.</div><div>I’ll send v5 with a changelog in the cover letter shortly.</div><br class=""><blockquote type="cite" class=""><div class=""><div class=""><br class="">Here is my diff, which I pushed on top of your branch. If you don't agree -<br class="">lets discuss. Otherwise squash and the patchset LGTM.<br class=""><br class="">================================================================================<br class=""><br class="">commit 7054ed8ffc5cff690858261073cdfb1822e241b7<br class="">Author: Vladislav Shpilevoy <<a href="mailto:v.shpilevoy@tarantool.org" class="">v.shpilevoy@tarantool.org</a>><br class="">Date: Fri Feb 28 00:02:10 2020 +0100<br class=""><br class=""> Review fixes<br class=""><br class="">diff --git a/src/box/wal.c b/src/box/wal.c<br class="">index bf127b259..1668c9348 100644<br class="">--- a/src/box/wal.c<br class="">+++ b/src/box/wal.c<br class="">@@ -278,11 +278,8 @@ tx_schedule_commit(struct cmsg *msg)<br class=""> <span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span>/* Closes the input valve. */<br class=""> <span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span>stailq_concat(&writer->rollback, &batch->rollback);<br class=""> <span class="Apple-tab-span" style="white-space:pre"> </span>}<br class="">-<br class="">-<span class="Apple-tab-span" style="white-space:pre"> </span>ERROR_INJECT(ERRINJ_REPLICASET_VCLOCK_UPDATE, { goto skip_update; });<br class=""> <span class="Apple-tab-span" style="white-space:pre"> </span>/* Update the tx vclock to the latest written by wal. */<br class=""> <span class="Apple-tab-span" style="white-space:pre"> </span>vclock_copy(&replicaset.vclock, &batch->vclock);<br class="">-<span class="Apple-tab-span" style="white-space:pre"> </span>ERROR_INJECT(ERRINJ_REPLICASET_VCLOCK_UPDATE, {skip_update:;});<br class=""> <span class="Apple-tab-span" style="white-space:pre"> </span>tx_schedule_queue(&batch->commit);<br class=""> <span class="Apple-tab-span" style="white-space:pre"> </span>mempool_free(&writer->msg_pool, container_of(msg, struct wal_msg, base));<br class=""> }<br class="">@@ -1117,6 +1114,7 @@ done:<br class=""> <span class="Apple-tab-span" style="white-space:pre"> </span>}<br class=""> <span class="Apple-tab-span" style="white-space:pre"> </span>fiber_gc();<br class=""> <span class="Apple-tab-span" style="white-space:pre"> </span>wal_notify_watchers(writer, WAL_EVENT_WRITE);<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span>ERROR_INJECT_SLEEP(ERRINJ_RELAY_FASTER_THAN_TX);<br class=""> }<br class=""><br class=""> /** WAL writer main loop. */<br class="">@@ -1328,6 +1326,8 @@ wal_watcher_notify(struct wal_watcher *watcher, unsigned events)<br class=""> <span class="Apple-tab-span" style="white-space:pre"> </span>msg->events = events;<br class=""> <span class="Apple-tab-span" style="white-space:pre"> </span>cmsg_init(&msg->cmsg, watcher->route);<br class=""> <span class="Apple-tab-span" style="white-space:pre"> </span>cpipe_push(&watcher->watcher_pipe, &msg->cmsg);<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span>ERROR_INJECT(ERRINJ_RELAY_FASTER_THAN_TX,<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span> cpipe_deliver_now(&watcher->watcher_pipe));<br class=""> }<br class=""><br class=""> static void<br class="">diff --git a/src/lib/core/cbus.h b/src/lib/core/cbus.h<br class="">index 16d122779..f0101cb8b 100644<br class="">--- a/src/lib/core/cbus.h<br class="">+++ b/src/lib/core/cbus.h<br class="">@@ -176,6 +176,13 @@ cpipe_set_max_input(struct cpipe *pipe, int max_input)<br class=""> <span class="Apple-tab-span" style="white-space:pre"> </span>pipe->max_input = max_input;<br class=""> }<br class=""><br class="">+static inline void<br class="">+cpipe_deliver_now(struct cpipe *pipe)<br class="">+{<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span>if (pipe->n_input > 0)<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span>ev_invoke(pipe->producer, &pipe->flush_input, EV_CUSTOM);<br class="">+}<br class="">+<br class=""> /**<br class=""> * Flush all staged messages into the pipe and eventually to the<br class=""> * consumer.<br class="">diff --git a/src/lib/core/errinj.h b/src/lib/core/errinj.h<br class="">index 58fe158fd..d8cdf3f27 100644<br class="">--- a/src/lib/core/errinj.h<br class="">+++ b/src/lib/core/errinj.h<br class="">@@ -137,7 +137,7 @@ struct errinj {<br class=""> <span class="Apple-tab-span" style="white-space:pre"> </span>_(ERRINJ_DYN_MODULE_COUNT, ERRINJ_INT, {.iparam = 0}) \<br class=""> <span class="Apple-tab-span" style="white-space:pre"> </span>_(ERRINJ_FIBER_MADVISE, ERRINJ_BOOL, {.bparam = false}) \<br class=""> <span class="Apple-tab-span" style="white-space:pre"> </span>_(ERRINJ_FIBER_MPROTECT, ERRINJ_INT, {.iparam = -1}) \<br class="">-<span class="Apple-tab-span" style="white-space:pre"> </span>_(ERRINJ_REPLICASET_VCLOCK_UPDATE, ERRINJ_BOOL, {.bparam = false}) \<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span>_(ERRINJ_RELAY_FASTER_THAN_TX, ERRINJ_BOOL, {.bparam = false}) \<br class=""><br class=""> ENUM0(errinj_id, ERRINJ_LIST);<br class=""> extern struct errinj errinjs[];<br class="">diff --git a/test/box/errinj.result b/test/box/errinj.result<br class="">index eb0905238..4ad24d0c1 100644<br class="">--- a/test/box/errinj.result<br class="">+++ b/test/box/errinj.result<br class="">@@ -59,12 +59,12 @@ evals<br class=""> - ERRINJ_PORT_DUMP: false<br class=""> - ERRINJ_RELAY_BREAK_LSN: -1<br class=""> - ERRINJ_RELAY_EXIT_DELAY: 0<br class="">+ - ERRINJ_RELAY_FASTER_THAN_TX: false<br class=""> - ERRINJ_RELAY_FINAL_JOIN: false<br class=""> - ERRINJ_RELAY_FINAL_SLEEP: false<br class=""> - ERRINJ_RELAY_REPORT_INTERVAL: 0<br class=""> - ERRINJ_RELAY_SEND_DELAY: false<br class=""> - ERRINJ_RELAY_TIMEOUT: 0<br class="">- - ERRINJ_REPLICASET_VCLOCK_UPDATE: false<br class=""> - ERRINJ_REPLICA_JOIN_DELAY: false<br class=""> - ERRINJ_SIO_READ_MAX: -1<br class=""> - ERRINJ_SNAP_COMMIT_DELAY: false<br class="">diff --git a/test/replication/gh-4739-vclock-assert.result b/test/replication/gh-4739-vclock-assert.result<br class="">index a612826a0..43d3f27f3 100644<br class="">--- a/test/replication/gh-4739-vclock-assert.result<br class="">+++ b/test/replication/gh-4739-vclock-assert.result<br class="">@@ -26,16 +26,19 @@ fiber = require('fiber')<br class=""> -- Stop updating replicaset vclock to simulate a situation, when<br class=""> -- a row is already relayed to the remote master, but the local<br class=""> -- vclock update hasn't happened yet.<br class="">-box.error.injection.set('ERRINJ_REPLICASET_VCLOCK_UPDATE', true)<br class="">+box.error.injection.set('ERRINJ_RELAY_FASTER_THAN_TX', true)<br class=""> | ---<br class=""> | - ok<br class=""> | ...<br class=""> lsn = box.info.lsn<br class=""> | ---<br class=""> | ...<br class="">-box.space._schema:replace{'something'}<br class="">+f = fiber.create(function() box.space._schema:replace{'something'} end)<br class=""> | ---<br class="">- | - ['something']<br class="">+ | ...<br class="">+test_run:wait_cond(function() return f:status() == 'suspended' end)<br class="">+ | ---<br class="">+ | - true<br class=""> | ...<br class=""></div></div></blockquote><div><br class=""></div>No need to call wait_cond() here. The fiber is suspended as soon as control is returned</div><div>to console, where we’re trying to call wait_cond.<br class=""><div><br class=""></div><span class="">diff --git a/test/replication/gh-4739-vclock-assert.result b/test/replication/gh-4739-vclock-assert.result<br class="">index 43d3f27f3..83896c4e1 100644<br class="">--- a/test/replication/gh-4739-vclock-assert.result<br class="">+++ b/test/replication/gh-4739-vclock-assert.result<br class="">@@ -36,9 +36,9 @@ lsn = box.info.lsn<br class=""> f = fiber.create(function() box.space._schema:replace{'something'} end)<br class=""> | ---<br class=""> | ...<br class="">-test_run:wait_cond(function() return f:status() == 'suspended' end)<br class="">+f:status()<br class=""> | ---<br class="">- | - true<br class="">+ | - suspended<br class=""> | ...<br class=""> -- Vclock isn't updated.<br class=""> box.info.lsn == lsn<br class=""></span><span class=""><br class=""></span><blockquote type="cite" class=""><div class=""><div class=""> -- Vclock isn't updated.<br class=""> box.info.lsn == lsn<br class="">@@ -53,7 +56,7 @@ end, 10)<br class=""><br class=""> -- Restart the remote instance. This will make the first instance<br class=""> -- resubscribe without entering orphan mode.<br class="">-test_run:cmd('restart server rebootstrap2')<br class="">+test_run:cmd('restart server rebootstrap2 with wait=False')<br class=""> | ---<br class=""> | - true<br class=""> | ...<br class="">@@ -68,10 +71,14 @@ end, 10)<br class=""> | ---<br class=""> | - true<br class=""> | ...<br class="">-box.error.injection.set('ERRINJ_REPLICASET_VCLOCK_UPDATE', false)<br class="">+box.error.injection.set('ERRINJ_RELAY_FASTER_THAN_TX', false)<br class=""> | ---<br class=""> | - ok<br class=""> | ...<br class="">+box.space._schema:get{'something'}<br class="">+ | ---<br class="">+ | - ['something']<br class="">+ | ...<br class=""> test_run:cmd('switch default')<br class=""> | ---<br class=""> | - true<br class="">diff --git a/test/replication/gh-4739-vclock-assert.test.lua b/test/replication/gh-4739-vclock-assert.test.lua<br class="">index b6a7caf3b..f8dd86688 100644<br class="">--- a/test/replication/gh-4739-vclock-assert.test.lua<br class="">+++ b/test/replication/gh-4739-vclock-assert.test.lua<br class="">@@ -10,9 +10,10 @@ fiber = require('fiber')<br class=""> -- Stop updating replicaset vclock to simulate a situation, when<br class=""> -- a row is already relayed to the remote master, but the local<br class=""> -- vclock update hasn't happened yet.<br class="">-box.error.injection.set('ERRINJ_REPLICASET_VCLOCK_UPDATE', true)<br class="">+box.error.injection.set('ERRINJ_RELAY_FASTER_THAN_TX', true)<br class=""> lsn = box.info.lsn<br class="">-box.space._schema:replace{'something'}<br class="">+f = fiber.create(function() box.space._schema:replace{'something'} end)<br class="">+test_run:wait_cond(function() return f:status() == 'suspended' end)<br class=""></div></div></blockquote><div style="margin: 0px; font-stretch: normal; font-size: 11px; line-height: normal; font-family: Menlo;" class=""><br class=""></div><span class="">diff --git a/test/replication/gh-4739-vclock-assert.test.lua b/test/replication/gh-4739-vclock-assert.test.lua<br class=""></span><span class="">index f8dd86688..5755ad752 100644<br class=""></span><span class="">--- a/test/replication/gh-4739-vclock-assert.test.lua<br class=""></span><span class="">+++ b/test/replication/gh-4739-vclock-assert.test.lua<br class=""></span><span class="">@@ -13,7 +13,7 @@ fiber = require('fiber')<br class=""></span><span class=""> box.error.injection.set('ERRINJ_RELAY_FASTER_THAN_TX', true)<br class=""></span><span class=""> lsn = box.info.lsn<br class=""></span><span class=""> f = fiber.create(function() box.space._schema:replace{'something'} end)<br class=""></span><span class="">-test_run:wait_cond(function() return f:status() == 'suspended' end)<br class=""></span><span class="">+f:status()<br class=""></span><span class=""> -- Vclock isn't updated.<br class=""></span><span class=""> box.info.lsn == lsn<br class=""></span><span class=""> <br class=""></span><span class=""><br class=""></span><blockquote type="cite" class=""><div class=""><div class=""> -- Vclock isn't updated.<br class=""> box.info.lsn == lsn<br class=""><br class="">@@ -23,12 +24,13 @@ end, 10)<br class=""><br class=""> -- Restart the remote instance. This will make the first instance<br class=""> -- resubscribe without entering orphan mode.<br class="">-test_run:cmd('restart server rebootstrap2')<br class="">+test_run:cmd('restart server rebootstrap2 with wait=False')<br class=""> test_run:cmd('switch rebootstrap1')<br class=""> -- Wait until resubscribe is sent<br class=""> test_run:wait_cond(function()\<br class=""> return box.info.replication[2].upstream.status == 'sync'\<br class=""> end, 10)<br class="">-box.error.injection.set('ERRINJ_REPLICASET_VCLOCK_UPDATE', false)<br class="">+box.error.injection.set('ERRINJ_RELAY_FASTER_THAN_TX', false)<br class="">+box.space._schema:get{'something'}<br class=""> test_run:cmd('switch default')<br class=""> test_run:drop_cluster(SERVERS)<br class=""></div></div></blockquote></div><br class=""><div class=""><br class=""></div><div class=""><div>--</div><div>Serge Petrenko</div><div><a href="mailto:sergepetrenko@tarantool.org" class="">sergepetrenko@tarantool.org</a></div></div><div class=""><br class=""></div></body></html>