Re: [Tarantool-patches] [PATCH v2 3/3] box: remove dead code from box_process_call/eval()

[Georgy Kirichenko <georgy@tarantool.org>]

[-- Attachment #1: Type: text/plain, Size: 2690 bytes --]

On Friday, 17 January 2020 00:54:23 MSK Vladislav Shpilevoy wrote:
> box_process_call/eval() in the end check if there is an
> active transaction. If there is, it is rolled back, and
> an error is set.
> 
> But rollback is not needed anymore, because anyway in
> the end of the request the fiber is stopped, and its
> not finished transaction is rolled back. Just setting
> of the error is enough.

Hi!
 Thanks for the patch, but I do not think that to remove an explicit rollback 
is a a good idea because of broken encapsulation - call and eval handlers 
should not rely on its execution context - a simple fiber, iproto fiber pool 
member or whatever else. Also I would like to mention that box_call and 
box_eval are members of the public api and it is not necessary that user will 
stop a fiber.

> 
> Follow-up #4662
> ---
>  src/box/call.c | 28 ++++------------------------
>  src/box/txn.c  |  1 +
>  2 files changed, 5 insertions(+), 24 deletions(-)
> 
> diff --git a/src/box/call.c b/src/box/call.c
> index bcaa453ea..a46a61c3c 100644
> --- a/src/box/call.c
> +++ b/src/box/call.c
> @@ -122,23 +122,13 @@ box_process_call(struct call_request *request, struct
> port *port) SC_FUNCTION, tt_cstr(name, name_len))) == 0) {
>  		rc = box_lua_call(name, name_len, &args, port);
>  	}
> -
> -	struct txn *txn = in_txn();
> -	if (rc != 0) {
> -		if (txn != NULL)
> -			txn_rollback(txn);
> -		fiber_gc();
> +	if (rc != 0)
>  		return -1;
> -	}
> -
> -	if (txn != NULL) {
> +	if (in_txn() != NULL) {
>  		diag_set(ClientError, ER_FUNCTION_TX_ACTIVE);
>  		port_destroy(port);
> -		txn_rollback(txn);
> -		fiber_gc();
>  		return -1;
>  	}
> -
>  	return 0;
>  }
> 
> @@ -154,21 +144,11 @@ box_process_eval(struct call_request *request, struct
> port *port) request->args_end - request->args);
>  	const char *expr = request->expr;
>  	uint32_t expr_len = mp_decode_strl(&expr);
> -	struct txn *txn;
> -	if (box_lua_eval(expr, expr_len, &args, port) != 0) {
> -		txn = in_txn();
> -		if (txn != NULL)
> -			txn_rollback(txn);
> -		fiber_gc();
> +	if (box_lua_eval(expr, expr_len, &args, port) != 0)
>  		return -1;
> -	}
> -
> -	txn = in_txn();
> -	if (txn != NULL) {
> +	if (in_txn() != 0) {
>  		diag_set(ClientError, ER_FUNCTION_TX_ACTIVE);
>  		port_destroy(port);
> -		txn_rollback(txn);
> -		fiber_gc();
>  		return -1;
>  	}
>  	return 0;
> diff --git a/src/box/txn.c b/src/box/txn.c
> index 963ec8eeb..0854bbacc 100644
> --- a/src/box/txn.c
> +++ b/src/box/txn.c
> @@ -845,6 +845,7 @@ txn_on_stop(struct trigger *trigger, void *event)
>  	(void) trigger;
>  	(void) event;
>  	txn_rollback(in_txn());                 /* doesn't yield or fail */
> +	fiber_gc();
>  	return 0;
>  }


[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 488 bytes --]