From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp52.i.mail.ru (smtp52.i.mail.ru [94.100.177.112]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 8E33046970E for ; Fri, 17 Jan 2020 20:41:49 +0300 (MSK) From: Georgy Kirichenko Date: Fri, 17 Jan 2020 20:41:48 +0300 Message-ID: <2144259.ElGaqSPkdT@localhost> In-Reply-To: <52419d26967890ace8245f46fdff0604f919a029.1579211601.git.v.shpilevoy@tarantool.org> References: <52419d26967890ace8245f46fdff0604f919a029.1579211601.git.v.shpilevoy@tarantool.org> MIME-Version: 1.0 Content-Type: multipart/signed; boundary="nextPart4506586.GXAFRqVoOG"; micalg="pgp-sha256"; protocol="application/pgp-signature" Subject: Re: [Tarantool-patches] [PATCH v2 3/3] box: remove dead code from box_process_call/eval() List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: tarantool-patches@dev.tarantool.org, kostja.osipov@gmail.com Cc: Vladislav Shpilevoy --nextPart4506586.GXAFRqVoOG Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" 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; > } --nextPart4506586.GXAFRqVoOG Content-Type: application/pgp-signature; name="signature.asc" Content-Description: This is a digitally signed message part. Content-Transfer-Encoding: 7Bit -----BEGIN PGP SIGNATURE----- iQEzBAABCAAdFiEECXG+Yw5ArYcP8x5wDVHWG5PoUw4FAl4h8dwACgkQDVHWG5Po Uw6CUwgAi3P3RxOx6b6NyCC6AeI+8yFX8XfAkXsYzd9PojoUfgvN8b0ulp/DWO0o kfadpzVx1k1YBWPuPe3ClE0WuN9q0gK2pBm/wqsH82VBEd5BnGd0dDWtWJU8jxaF jqpfCqvSRNetiL+D4PP4NbTH3dALOpH039purTCg2UV+lVblIMxLRBT2iZdHy6P+ atbIkPZuOGpuOXNY8kiEt/z6HSnZojJqBASgDrnz8Z/uaDQhJ5hvlZ5zNhCr/brF po1OD4qfPvejbrMQ48CrwKYNzlMoj0zcB+OFiQ8eYmoe+8r3/erH9k8BxAKzhDO5 YexGIcTvWE/uI1qgAmm2/gozXR04KA== =uDFr -----END PGP SIGNATURE----- --nextPart4506586.GXAFRqVoOG--