[PATCH 1/2] vinyl: drop useless vy_read_view->is_aborted flag

[PATCH 1/2] vinyl: drop useless vy_read_view->is_aborted flag

[Vladimir Davydov]

It is supposed to abort an iterator that read a statement reverted after
a failed WAL write. However, we will abort such an iterator anyway - see
vy_tx_abort_readers(). So let's drop the useless flag.

It turned out that we never tested such a case (try grepping "read view
is aborted" error). So let's also add a functional test for it.
---
https://github.com/tarantool/tarantool/issues/4070
https://github.com/tarantool/tarantool/commits/dv/gh-4070-vy-fail-aborted-transactions-early

 src/box/vinyl.c               |  2 +-
 src/box/vy_read_view.h        |  6 -----
 src/box/vy_tx.c               |  6 -----
 test/vinyl/errinj_tx.result   | 60 +++++++++++++++++++++++++++++++++++++++++++
 test/vinyl/errinj_tx.test.lua | 28 ++++++++++++++++++++
 5 files changed, 89 insertions(+), 13 deletions(-)

diff --git a/src/box/vinyl.c b/src/box/vinyl.c
index c2b7eb78..bf27f5fb 100644
--- a/src/box/vinyl.c
+++ b/src/box/vinyl.c
@@ -3716,7 +3716,7 @@ vinyl_iterator_check_tx(struct vinyl_iterator *it)
 		diag_set(ClientError, ER_CURSOR_NO_TRANSACTION);
 		return -1;
 	}
-	if (it->tx->state == VINYL_TX_ABORT || it->tx->read_view->is_aborted) {
+	if (it->tx->state == VINYL_TX_ABORT) {
 		/* Transaction read view was aborted. */
 		diag_set(ClientError, ER_READ_VIEW_ABORTED);
 		return -1;
diff --git a/src/box/vy_read_view.h b/src/box/vy_read_view.h
index c3b14699..a9b37dcc 100644
--- a/src/box/vy_read_view.h
+++ b/src/box/vy_read_view.h
@@ -63,12 +63,6 @@ struct vy_read_view {
 	 * count it as it is missing from read_views list.
 	 */
 	int refs;
-	/**
-	 * Is set to true when the read view which includes
-	 * a prepared but not committed transaction, is
-	 * compromised by a cascading rollback.
-	 */
-	bool is_aborted;
 };
 
 #if defined(__cplusplus)
diff --git a/src/box/vy_tx.c b/src/box/vy_tx.c
index c4445505..ab0bf071 100644
--- a/src/box/vy_tx.c
+++ b/src/box/vy_tx.c
@@ -97,7 +97,6 @@ vy_global_read_view_create(struct vy_read_view *rv, int64_t lsn)
 	 */
 	rv->vlsn = lsn;
 	rv->refs = 0;
-	rv->is_aborted = false;
 }
 
 struct tx_manager *
@@ -185,7 +184,6 @@ tx_manager_read_view(struct tx_manager *xm)
 			 "mempool", "read view");
 		return NULL;
 	}
-	rv->is_aborted = false;
 	if (xm->last_prepared_tx != NULL) {
 		rv->vlsn = MAX_LSN + xm->last_prepared_tx->psn;
 		xm->last_prepared_tx->read_view = rv;
@@ -815,10 +813,6 @@ vy_tx_rollback_after_prepare(struct vy_tx *tx)
 			vy_mem_unpin(v->mem);
 	}
 
-	/* Abort read views of dependent transactions. */
-	if (tx->read_view != &xm->global_read_view)
-		tx->read_view->is_aborted = true;
-
 	struct write_set_iterator it;
 	write_set_ifirst(&tx->write_set, &it);
 	while ((v = write_set_inext(&it)) != NULL) {
diff --git a/test/vinyl/errinj_tx.result b/test/vinyl/errinj_tx.result
index 29ec47c8..e2e4916e 100644
--- a/test/vinyl/errinj_tx.result
+++ b/test/vinyl/errinj_tx.result
@@ -433,6 +433,66 @@ last_read
 space:drop()
 ---
 ...
+--
+-- Check that dependent transaction is aborted on WAL write.
+--
+s = box.schema.space.create('test', {engine = 'vinyl'})
+---
+...
+_ = s:create_index('pk')
+---
+...
+s:replace{10}
+---
+- [10]
+...
+-- Insert a tuple into the memory level and stall on WAL write.
+ch = fiber.channel(1)
+---
+...
+errinj.set('ERRINJ_WAL_DELAY', true)
+---
+- ok
+...
+_ = fiber.create(function() pcall(s.replace, s, {1}) ch:put(true) end)
+---
+...
+-- Read the tuple from another transaction.
+itr = create_iterator(s)
+---
+...
+itr.next()
+---
+- [1]
+...
+-- Resume the first transaction and let it fail on WAL write.
+errinj.set('ERRINJ_WAL_WRITE', true)
+---
+- ok
+...
+errinj.set('ERRINJ_WAL_DELAY', false)
+---
+- ok
+...
+ch:get()
+---
+- true
+...
+errinj.set('ERRINJ_WAL_WRITE', false)
+---
+- ok
+...
+-- Must fail.
+itr.next()
+---
+- error: The read view is aborted
+...
+itr = nil
+---
+...
+s:drop()
+---
+...
 -- Collect all iterators to make sure no read views are left behind,
 -- as they might disrupt the following test run.
 collectgarbage()
diff --git a/test/vinyl/errinj_tx.test.lua b/test/vinyl/errinj_tx.test.lua
index b06831b7..d17c1abb 100644
--- a/test/vinyl/errinj_tx.test.lua
+++ b/test/vinyl/errinj_tx.test.lua
@@ -193,6 +193,34 @@ last_read
 
 space:drop()
 
+--
+-- Check that dependent transaction is aborted on WAL write.
+--
+s = box.schema.space.create('test', {engine = 'vinyl'})
+_ = s:create_index('pk')
+s:replace{10}
+
+-- Insert a tuple into the memory level and stall on WAL write.
+ch = fiber.channel(1)
+errinj.set('ERRINJ_WAL_DELAY', true)
+_ = fiber.create(function() pcall(s.replace, s, {1}) ch:put(true) end)
+
+-- Read the tuple from another transaction.
+itr = create_iterator(s)
+itr.next()
+
+-- Resume the first transaction and let it fail on WAL write.
+errinj.set('ERRINJ_WAL_WRITE', true)
+errinj.set('ERRINJ_WAL_DELAY', false)
+ch:get()
+errinj.set('ERRINJ_WAL_WRITE', false)
+
+-- Must fail.
+itr.next()
+
+itr = nil
+s:drop()
+
 -- Collect all iterators to make sure no read views are left behind,
 -- as they might disrupt the following test run.
 collectgarbage()
-- 
2.11.0

[PATCH 2/2] vinyl: fail aborted transactions early

[Vladimir Davydov]

We abort transactions when switching to read-only mode, building a new
index, or reverting a statement after a failed WAL write. It's no use
to allow an aborted transaction to proceed as usual until commit - we
should fail it as early as we can to avoid wasting resources. Currently,
we do it in a rather abrupt way - by an assertion :-) This patch makes
any DML/DQL operation fail gracefully instead.

Closes #4070
---
https://github.com/tarantool/tarantool/issues/4070
https://github.com/tarantool/tarantool/commits/dv/gh-4070-vy-fail-aborted-transactions-early

 src/box/vinyl.c                | 16 +++++++++++----
 src/box/vy_tx.c                | 40 ++++++++++++++++++++++++++++---------
 src/box/vy_tx.h                |  4 ++--
 test/vinyl/errinj_ddl.result   | 45 ++++++++++++++++++++++++++++++++++++++++++
 test/vinyl/errinj_ddl.test.lua | 20 +++++++++++++++++++
 test/vinyl/errinj_tx.result    | 29 +++++++++++++++++++++++++++
 test/vinyl/errinj_tx.test.lua  | 10 ++++++++++
 test/vinyl/misc.result         | 20 +++++++++++++++++++
 test/vinyl/misc.test.lua       | 11 +++++++++++
 9 files changed, 180 insertions(+), 15 deletions(-)

diff --git a/src/box/vinyl.c b/src/box/vinyl.c
index bf27f5fb..3ef43e18 100644
--- a/src/box/vinyl.c
+++ b/src/box/vinyl.c
@@ -2434,8 +2434,7 @@ vinyl_engine_begin_statement(struct engine *engine, struct txn *txn)
 	struct vy_tx *tx = txn->engine_tx;
 	struct txn_stmt *stmt = txn_current_stmt(txn);
 	assert(tx != NULL);
-	stmt->engine_savepoint = vy_tx_begin_statement(tx);
-	return 0;
+	return vy_tx_begin_statement(tx, &stmt->engine_savepoint);
 }
 
 static void
@@ -3817,6 +3816,12 @@ vinyl_index_create_iterator(struct index *base, enum iterator_type type,
 		return NULL;
 	}
 
+	struct vy_tx *tx = in_txn() ? in_txn()->engine_tx : NULL;
+	if (tx != NULL && tx->state == VINYL_TX_ABORT) {
+		diag_set(ClientError, ER_TRANSACTION_CONFLICT);
+		return NULL;
+	}
+
 	struct vinyl_iterator *it = mempool_alloc(&env->iterator_pool);
 	if (it == NULL) {
 	        diag_set(OutOfMemory, sizeof(struct vinyl_iterator),
@@ -3840,8 +3845,6 @@ vinyl_index_create_iterator(struct index *base, enum iterator_type type,
 	it->lsm = lsm;
 	vy_lsm_ref(lsm);
 
-	struct vy_tx *tx = in_txn() ? in_txn()->engine_tx : NULL;
-	assert(tx == NULL || tx->state == VINYL_TX_READY);
 	if (tx != NULL) {
 		/*
 		 * Register a trigger that will abort this iterator
@@ -3874,6 +3877,11 @@ vinyl_index_get(struct index *index, const char *key,
 	const struct vy_read_view **rv = (tx != NULL ? vy_tx_read_view(tx) :
 					  &env->xm->p_global_read_view);
 
+	if (tx != NULL && tx->state == VINYL_TX_ABORT) {
+		diag_set(ClientError, ER_TRANSACTION_CONFLICT);
+		return -1;
+	}
+
 	if (vy_get_by_raw_key(lsm, tx, rv, key, part_count, ret) != 0)
 		return -1;
 	if (*ret != NULL) {
diff --git a/src/box/vy_tx.c b/src/box/vy_tx.c
index ab0bf071..ae660dd8 100644
--- a/src/box/vy_tx.c
+++ b/src/box/vy_tx.c
@@ -335,6 +335,15 @@ vy_tx_destroy(struct vy_tx *tx)
 	rlist_del_entry(tx, in_writers);
 }
 
+/** Mark a transaction as aborted and account it in stats. */
+static void
+vy_tx_abort(struct vy_tx *tx)
+{
+	assert(tx->state == VINYL_TX_READY);
+	tx->state = VINYL_TX_ABORT;
+	tx->xm->stat.conflict++;
+}
+
 /** Return true if the transaction is read-only. */
 static bool
 vy_tx_is_ro(struct vy_tx *tx)
@@ -394,7 +403,7 @@ vy_tx_abort_readers(struct vy_tx *tx, struct txv *v)
 		/* Abort only active TXs */
 		if (abort->state != VINYL_TX_READY)
 			continue;
-		abort->state = VINYL_TX_ABORT;
+		vy_tx_abort(abort);
 	}
 }
 
@@ -614,13 +623,18 @@ vy_tx_prepare(struct vy_tx *tx)
 {
 	struct tx_manager *xm = tx->xm;
 
+	if (tx->state == VINYL_TX_ABORT) {
+		/* Conflict is already accounted - see vy_tx_abort(). */
+		diag_set(ClientError, ER_TRANSACTION_CONFLICT);
+		return -1;
+	}
+
+	assert(tx->state == VINYL_TX_READY);
 	if (vy_tx_is_ro(tx)) {
-		assert(tx->state == VINYL_TX_READY);
 		tx->state = VINYL_TX_COMMIT;
 		return 0;
 	}
-
-	if (vy_tx_is_in_read_view(tx) || tx->state == VINYL_TX_ABORT) {
+	if (vy_tx_is_in_read_view(tx)) {
 		xm->stat.conflict++;
 		diag_set(ClientError, ER_TRANSACTION_CONFLICT);
 		return -1;
@@ -834,18 +848,26 @@ vy_tx_rollback(struct vy_tx *tx)
 	mempool_free(&xm->tx_mempool, tx);
 }
 
-void *
-vy_tx_begin_statement(struct vy_tx *tx)
+int
+vy_tx_begin_statement(struct vy_tx *tx, void **savepoint)
 {
+	if (tx->state == VINYL_TX_ABORT) {
+		diag_set(ClientError, ER_TRANSACTION_CONFLICT);
+		return -1;
+	}
 	assert(tx->state == VINYL_TX_READY);
 	if (stailq_empty(&tx->log))
 		rlist_add_entry(&tx->xm->writers, tx, in_writers);
-	return stailq_last(&tx->log);
+	*savepoint = stailq_last(&tx->log);
+	return 0;
 }
 
 void
 vy_tx_rollback_statement(struct vy_tx *tx, void *svp)
 {
+	if (tx->state == VINYL_TX_ABORT)
+		return;
+
 	assert(tx->state == VINYL_TX_READY);
 	struct stailq_entry *last = svp;
 	struct stailq tail;
@@ -1071,7 +1093,7 @@ tx_manager_abort_writers_for_ddl(struct tx_manager *xm, struct vy_lsm *lsm)
 		if (tx->state == VINYL_TX_READY &&
 		    write_set_search_key(&tx->write_set, lsm,
 					 lsm->env->empty_key) != NULL)
-			tx->state = VINYL_TX_ABORT;
+			vy_tx_abort(tx);
 	}
 }
 
@@ -1082,7 +1104,7 @@ tx_manager_abort_writers_for_ro(struct tx_manager *xm)
 	rlist_foreach_entry(tx, &xm->writers, in_writers) {
 		/* Applier ignores ro flag. */
 		if (tx->state == VINYL_TX_READY && !tx->is_applier_session)
-			tx->state = VINYL_TX_ABORT;
+			vy_tx_abort(tx);
 	}
 }
 
diff --git a/src/box/vy_tx.h b/src/box/vy_tx.h
index 79fb9ac2..aaa31bee 100644
--- a/src/box/vy_tx.h
+++ b/src/box/vy_tx.h
@@ -326,8 +326,8 @@ vy_tx_rollback(struct vy_tx *tx);
  * transaction state. The transaction can be rolled back
  * to a save point with vy_tx_rollback_statement().
  */
-void *
-vy_tx_begin_statement(struct vy_tx *tx);
+int
+vy_tx_begin_statement(struct vy_tx *tx, void **savepoint);
 
 /**
  * Rollback a transaction statement.
diff --git a/test/vinyl/errinj_ddl.result b/test/vinyl/errinj_ddl.result
index 932132db..04cb581f 100644
--- a/test/vinyl/errinj_ddl.result
+++ b/test/vinyl/errinj_ddl.result
@@ -637,3 +637,48 @@ c:get()
 s:drop()
 ---
 ...
+--
+-- gh-4070: a transaction aborted by DDL must fail any DML/DQL request.
+--
+s = box.schema.space.create('test', {engine = 'vinyl'})
+---
+...
+_ = s:create_index('pk')
+---
+...
+s:replace{1, 1}
+---
+- [1, 1]
+...
+box.begin()
+---
+...
+s:replace{1, 2}
+---
+- [1, 2]
+...
+ch = fiber.channel(1)
+---
+...
+fiber = fiber.create(function() s:create_index('sk', {parts = {2, 'unsigned'}}) ch:put(true) end)
+---
+...
+ch:get()
+---
+- true
+...
+s:get(1)
+---
+- error: Transaction has been aborted by conflict
+...
+s:replace{1, 3}
+---
+- error: Transaction has been aborted by conflict
+...
+box.commit()
+---
+- error: Transaction has been aborted by conflict
+...
+s:drop()
+---
+...
diff --git a/test/vinyl/errinj_ddl.test.lua b/test/vinyl/errinj_ddl.test.lua
index 95e0ad3c..50dc4aa1 100644
--- a/test/vinyl/errinj_ddl.test.lua
+++ b/test/vinyl/errinj_ddl.test.lua
@@ -278,3 +278,23 @@ errinj.set('ERRINJ_VY_READ_PAGE_TIMEOUT', 0)
 c:get()
 
 s:drop()
+
+--
+-- gh-4070: a transaction aborted by DDL must fail any DML/DQL request.
+--
+s = box.schema.space.create('test', {engine = 'vinyl'})
+_ = s:create_index('pk')
+s:replace{1, 1}
+
+box.begin()
+s:replace{1, 2}
+
+ch = fiber.channel(1)
+fiber = fiber.create(function() s:create_index('sk', {parts = {2, 'unsigned'}}) ch:put(true) end)
+ch:get()
+
+s:get(1)
+s:replace{1, 3}
+box.commit()
+
+s:drop()
diff --git a/test/vinyl/errinj_tx.result b/test/vinyl/errinj_tx.result
index e2e4916e..7d59d156 100644
--- a/test/vinyl/errinj_tx.result
+++ b/test/vinyl/errinj_tx.result
@@ -436,6 +436,8 @@ space:drop()
 --
 -- Check that dependent transaction is aborted on WAL write.
 --
+-- gh-4070: an aborted transaction must fail any DML/DQL request.
+--
 s = box.schema.space.create('test', {engine = 'vinyl'})
 ---
 ...
@@ -465,6 +467,17 @@ itr.next()
 ---
 - [1]
 ...
+c = txn_proxy.new()
+---
+...
+c:begin()
+---
+- 
+...
+c('s:get(1)')
+---
+- - [1]
+...
 -- Resume the first transaction and let it fail on WAL write.
 errinj.set('ERRINJ_WAL_WRITE', true)
 ---
@@ -487,6 +500,22 @@ itr.next()
 ---
 - error: The read view is aborted
 ...
+c('s:get(1)')
+---
+- - {'error': 'Transaction has been aborted by conflict'}
+...
+c('s:select()')
+---
+- - {'error': 'Transaction has been aborted by conflict'}
+...
+c('s:replace{1}')
+---
+- - {'error': 'Transaction has been aborted by conflict'}
+...
+c:commit()
+---
+- - {'error': 'Transaction has been aborted by conflict'}
+...
 itr = nil
 ---
 ...
diff --git a/test/vinyl/errinj_tx.test.lua b/test/vinyl/errinj_tx.test.lua
index d17c1abb..c434d92f 100644
--- a/test/vinyl/errinj_tx.test.lua
+++ b/test/vinyl/errinj_tx.test.lua
@@ -196,6 +196,8 @@ space:drop()
 --
 -- Check that dependent transaction is aborted on WAL write.
 --
+-- gh-4070: an aborted transaction must fail any DML/DQL request.
+--
 s = box.schema.space.create('test', {engine = 'vinyl'})
 _ = s:create_index('pk')
 s:replace{10}
@@ -209,6 +211,10 @@ _ = fiber.create(function() pcall(s.replace, s, {1}) ch:put(true) end)
 itr = create_iterator(s)
 itr.next()
 
+c = txn_proxy.new()
+c:begin()
+c('s:get(1)')
+
 -- Resume the first transaction and let it fail on WAL write.
 errinj.set('ERRINJ_WAL_WRITE', true)
 errinj.set('ERRINJ_WAL_DELAY', false)
@@ -217,6 +223,10 @@ errinj.set('ERRINJ_WAL_WRITE', false)
 
 -- Must fail.
 itr.next()
+c('s:get(1)')
+c('s:select()')
+c('s:replace{1}')
+c:commit()
 
 itr = nil
 s:drop()
diff --git a/test/vinyl/misc.result b/test/vinyl/misc.result
index 685fd776..4f613cb0 100644
--- a/test/vinyl/misc.result
+++ b/test/vinyl/misc.result
@@ -286,6 +286,8 @@ test_run:cmd('cleanup server test')
 -- gh-4016: local rw transactions are aborted when the instance
 -- switches to read-only mode.
 --
+-- gh-4070: an aborted transaction must fail any DML/DQL request.
+--
 s = box.schema.space.create('test', {engine = 'vinyl'})
 ---
 ...
@@ -308,6 +310,10 @@ _ = fiber.create(function()
     box.begin()
     s:replace{1, 2}
     ch1:get()
+    local status, err = pcall(s.replace, s, {3, 4})
+    ch1:put(status or err)
+    local status, err = pcall(s.select, s)
+    ch1:put(status or err)
     local status, err = pcall(box.commit)
     ch1:put(status or err)
 end);
@@ -321,6 +327,8 @@ _ = fiber.create(function()
     box.begin()
     s:select()
     ch2:get()
+    local status, err = pcall(s.select, s)
+    ch2:put(status or err)
     local status, err = pcall(box.commit)
     ch2:put(status or err)
 end);
@@ -345,8 +353,20 @@ ch2:put(true)
 ...
 ch1:get()
 ---
+- Can't modify data because this instance is in read-only mode.
+...
+ch1:get()
+---
 - Transaction has been aborted by conflict
 ...
+ch1:get()
+---
+- Transaction has been aborted by conflict
+...
+ch2:get()
+---
+- true
+...
 ch2:get()
 ---
 - true
diff --git a/test/vinyl/misc.test.lua b/test/vinyl/misc.test.lua
index cdc22774..bffaaf16 100644
--- a/test/vinyl/misc.test.lua
+++ b/test/vinyl/misc.test.lua
@@ -119,6 +119,8 @@ test_run:cmd('cleanup server test')
 -- gh-4016: local rw transactions are aborted when the instance
 -- switches to read-only mode.
 --
+-- gh-4070: an aborted transaction must fail any DML/DQL request.
+--
 s = box.schema.space.create('test', {engine = 'vinyl'})
 _ = s:create_index('pk')
 s:replace({1, 1})
@@ -129,6 +131,10 @@ _ = fiber.create(function()
     box.begin()
     s:replace{1, 2}
     ch1:get()
+    local status, err = pcall(s.replace, s, {3, 4})
+    ch1:put(status or err)
+    local status, err = pcall(s.select, s)
+    ch1:put(status or err)
     local status, err = pcall(box.commit)
     ch1:put(status or err)
 end);
@@ -138,6 +144,8 @@ _ = fiber.create(function()
     box.begin()
     s:select()
     ch2:get()
+    local status, err = pcall(s.select, s)
+    ch2:put(status or err)
     local status, err = pcall(box.commit)
     ch2:put(status or err)
 end);
@@ -148,6 +156,9 @@ box.cfg{read_only = true}
 ch1:put(true)
 ch2:put(true)
 ch1:get()
+ch1:get()
+ch1:get()
+ch2:get()
 ch2:get()
 -- Cleanup.
 box.cfg{read_only = false}
-- 
2.11.0

[tarantool-patches] Re: [PATCH 1/2] vinyl: drop useless vy_read_view->is_aborted flag

[Konstantin Osipov]

* Vladimir Davydov <vdavydov.dev@gmail.com> [19/03/22 21:17]:
> It is supposed to abort an iterator that read a statement reverted after
> a failed WAL write. However, we will abort such an iterator anyway - see
> vy_tx_abort_readers(). So let's drop the useless flag.
> 
> It turned out that we never tested such a case (try grepping "read view
> is aborted" error). So let's also add a functional test for it.

OK to push.


-- 
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.io - www.twitter.com/kostja_osipov

[tarantool-patches] Re: [PATCH 2/2] vinyl: fail aborted transactions early

[Konstantin Osipov]

* Vladimir Davydov <vdavydov.dev@gmail.com> [19/03/22 21:17]:
> We abort transactions when switching to read-only mode, building a new
> index, or reverting a statement after a failed WAL write. It's no use
> to allow an aborted transaction to proceed as usual until commit - we
> should fail it as early as we can to avoid wasting resources. Currently,
> we do it in a rather abrupt way - by an assertion :-) This patch makes
> any DML/DQL operation fail gracefully instead.
> Closes #4070

OK to push.
-- 
konstantin osipov, moscow, russia, +7 903 626 22 32
http://tarantool.io - www.twitter.com/kostja_osipov

Re: [tarantool-patches] Re: [PATCH 2/2] vinyl: fail aborted transactions early

[Vladimir Davydov]

On Thu, Mar 28, 2019 at 05:05:57PM +0300, Konstantin Osipov wrote:
> * Vladimir Davydov <vdavydov.dev@gmail.com> [19/03/22 21:17]:
> > We abort transactions when switching to read-only mode, building a new
> > index, or reverting a statement after a failed WAL write. It's no use
> > to allow an aborted transaction to proceed as usual until commit - we
> > should fail it as early as we can to avoid wasting resources. Currently,
> > we do it in a rather abrupt way - by an assertion :-) This patch makes
> > any DML/DQL operation fail gracefully instead.
> > Closes #4070
> 
> OK to push.

Pushed both patches to master, 2.1, and 1.10.