[tarantool-patches] Re: [PATCH v4 02/20] refactoring: wrap incorrigibly throwing calls in try..catch block

[Sergey Ostanevich]

LGTM, v5 means a patch #21 will contain all necessary follow-ups.

On 21 Oct 14:32, Ilya Kosarev wrote:
> 
> Hi!
> 
> Thanks for your review.
> In v5 I will add new patch to this patchset to wrap  std-originated  exceptions.
> 
> Sincerely,
> Ilya Kosarev
> 
> 
> >Вторник,  8 октября 2019, 22:53 +03:00 от Sergey Ostanevich <sergos at tarantool.org>:
> >
> >Hi Ilya!
> >
> >Thanks for the patch.
> >Please, put into the follow-up list fixes for std:: sources of
> >exceptions, such as std::bad_alloc that can appear in
> >on_replace_dd_space() at alter.cc:2135 and in functions called from it.
> >The same is applicalbe to all functions you changed.
> >
> >With the above taken into account - LGTM.
> >
> >Sergos
> >
> >On 23 Sep 18:56, Ilya Kosarev wrote:
> >> Some functions called from triggers won't be cleared from
> >> exceptions within this patchset. They are wrapped in try..catch
> >> blocks.
> >> 
> >> Part of #4247
> >> ---
> >>  src/box/alter.cc       | 58 ++++++++++++++++++++++++++++++++++--------
> >>  src/box/replication.cc | 12 ++++++---
> >>  2 files changed, 55 insertions(+), 15 deletions(-)
> >> 
> >> diff --git a/src/box/alter.cc b/src/box/alter.cc
> >> index e21dce5bf..2c1c31023 100644
> >> --- a/src/box/alter.cc
> >> +++ b/src/box/alter.cc
> >> @@ -878,8 +878,12 @@ alter_space_commit(struct trigger *trigger, void *event)
> >>  	 * indexes into their new places.
> >>  	 */
> >>  	class AlterSpaceOp *op;
> >> -	rlist_foreach_entry(op, &alter->ops, link)
> >> -		op->commit(alter, signature);
> >> +	try {
> >> +		rlist_foreach_entry(op, &alter->ops, link)
> >> +			op->commit(alter, signature);
> >> +	} catch (Exception *e) {
> >> +		return -1;
> >> +	}
> >> 
> >>  	alter->new_space = NULL; /* for alter_space_delete(). */
> >>  	/*
> >> @@ -906,8 +910,12 @@ alter_space_rollback(struct trigger *trigger, void * /* event */)
> >>  	struct alter_space *alter = (struct alter_space *) trigger->data;
> >>  	/* Rollback alter ops */
> >>  	class AlterSpaceOp *op;
> >> -	rlist_foreach_entry(op, &alter->ops, link) {
> >> -		op->rollback(alter);
> >> +	try {
> >> +		rlist_foreach_entry(op, &alter->ops, link) {
> >> +			op->rollback(alter);
> >> +		}
> >> +	} catch (Exception *e) {
> >> +		return -1;
> >>  	}
> >>  	/* Rebuild index maps once for all indexes. */
> >>  	space_fill_index_map(alter->old_space);
> >> @@ -2132,7 +2140,11 @@ on_replace_dd_space(struct trigger * /* trigger */, void *event)
> >>  		alter_space_move_indexes(alter, 0, old_space->index_id_max + 1);
> >>  		/* Remember to update schema_version. */
> >>  		(void) new UpdateSchemaVersion(alter);
> >> -		alter_space_do(stmt, alter);
> >> +		try {
> >> +			alter_space_do(stmt, alter);
> >> +		} catch (Exception *e) {
> >> +			return -1;
> >> +		}
> >>  		alter_guard.is_active = false;
> >>  	}
> >>  	return 0;
> >> @@ -2371,7 +2383,11 @@ on_replace_dd_index(struct trigger * /* trigger */, void *event)
> >>  	(void) new MoveCkConstraints(alter);
> >>  	/* Add an op to update schema_version on commit. */
> >>  	(void) new UpdateSchemaVersion(alter);
> >> -	alter_space_do(stmt, alter);
> >> +	try {
> >> +		alter_space_do(stmt, alter);
> >> +	} catch (Exception *e) {
> >> +		return -1;
> >> +	}
> >>  	scoped_guard.is_active = false;
> >>  	return 0;
> >>  }
> >> @@ -2448,7 +2464,11 @@ on_replace_dd_truncate(struct trigger * /* trigger */, void *event)
> >>  	}
> >> 
> >>  	(void) new MoveCkConstraints(alter);
> >> -	alter_space_do(stmt, alter);
> >> +	try {
> >> +		alter_space_do(stmt, alter);
> >> +	} catch (Exception *e) {
> >> +		return -1;
> >> +	}
> >>  	scoped_guard.is_active = false;
> >>  	return 0;
> >>  }
> >> @@ -2611,7 +2631,11 @@ user_cache_alter_user(struct trigger *trigger, void * /* event */)
> >>  	struct user_def *user = user_def_new_from_tuple(tuple);
> >>  	auto def_guard = make_scoped_guard([=] { free(user); });
> >>  	/* Can throw if, e.g. too many users. */
> >> -	user_cache_replace(user);
> >> +	try {
> >> +		user_cache_replace(user);
> >> +	} catch (Exception *e) {
> >> +		return -1;
> >> +	}
> >>  	def_guard.is_active = false;
> >>  	return 0;
> >>  }
> >> @@ -2635,7 +2659,11 @@ on_replace_dd_user(struct trigger * /* trigger */, void *event)
> >>  		access_check_ddl(user->name, user->uid, user->owner, user->type,
> >>  				 PRIV_C);
> >>  		auto def_guard = make_scoped_guard([=] { free(user); });
> >> -		(void) user_cache_replace(user);
> >> +		try {
> >> +			(void) user_cache_replace(user);
> >> +		} catch (Exception *e) {
> >> +			return -1;
> >> +		}
> >>  		def_guard.is_active = false;
> >>  		struct trigger *on_rollback =
> >>  			txn_alter_trigger_new(user_cache_remove_user, new_tuple);
> >> @@ -2673,7 +2701,11 @@ on_replace_dd_user(struct trigger * /* trigger */, void *event)
> >>  		access_check_ddl(user->name, user->uid, user->uid,
> >>  			         old_user->def->type, PRIV_A);
> >>  		auto def_guard = make_scoped_guard([=] { free(user); });
> >> -		user_cache_replace(user);
> >> +		try {
> >> +			user_cache_replace(user);
> >> +		} catch (Exception *e) {
> >> +			return -1;
> >> +		}
> >>  		def_guard.is_active = false;
> >>  		struct trigger *on_rollback =
> >>  			txn_alter_trigger_new(user_cache_alter_user, old_tuple);
> >> @@ -4872,7 +4904,11 @@ on_replace_dd_func_index(struct trigger *trigger, void *event)
> >>  				 space->index_id_max + 1);
> >>  	(void) new MoveCkConstraints(alter);
> >>  	(void) new UpdateSchemaVersion(alter);
> >> -	alter_space_do(stmt, alter);
> >> +	try {
> >> +		alter_space_do(stmt, alter);
> >> +	} catch (Exception *e) {
> >> +		return -1;
> >> +	}
> >> 
> >>  	scoped_guard.is_active = false;
> >>  	return 0;
> >> diff --git a/src/box/replication.cc b/src/box/replication.cc
> >> index 030221457..82bf496c8 100644
> >> --- a/src/box/replication.cc
> >> +++ b/src/box/replication.cc
> >> @@ -415,10 +415,14 @@ replica_on_applier_state_f(struct trigger *trigger, void *event)
> >>  		replicaset.is_joining = false;
> >>  		break;
> >>  	case APPLIER_CONNECTED:
> >> -		if (tt_uuid_is_nil(&replica->uuid))
> >> -			replica_on_applier_connect(replica);
> >> -		else
> >> -			replica_on_applier_reconnect(replica);
> >> +		try {
> >> +			if (tt_uuid_is_nil(&replica->uuid))
> >> +				replica_on_applier_connect(replica);
> >> +			else
> >> +				replica_on_applier_reconnect(replica);
> >> +		} catch (Exception *e) {
> >> +			return -1;
> >> +		}
> >>  		break;
> >>  	case APPLIER_LOADING:
> >>  	case APPLIER_DISCONNECTED:
> >> -- 
> >> 2.17.1
> >> 
> >> 
> >
> 
> 
> -- 
> Ilya Kosarev