From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp5.mail.ru (smtp5.mail.ru [94.100.179.24]) (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 22C17469710 for ; Thu, 7 May 2020 03:36:18 +0300 (MSK) Date: Thu, 7 May 2020 00:36:17 +0000 From: Nikita Pettik Message-ID: <20200507003617.GB9992@tarantool.org> References: <7965217ceed66d448cee453c690e8d91ba7a841b.1587948306.git.korablev@tarantool.org> <653509c4-55b0-a7cb-55ab-eb534a0e6421@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <653509c4-55b0-a7cb-55ab-eb534a0e6421@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH v3 3/3] vinyl: clean-up write iterator if vy_task_write_run() fails List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Aleksandr Lyapunov Cc: tarantool-patches@dev.tarantool.org, v.shpilevoy@tarantool.org On 06 May 13:37, Aleksandr Lyapunov wrote: > It's very strange, as I see start() method already makes its own cleanup > in case of failure and does not require 'stop' call in this case. > If it does not do it correctly - it must be fixed (or removed completely, > but it seems to be very serious change). Turns out start() doesn't provide proper clean-up: in case vy_write_iterator_add_src() fails, only last source is cleaned up, meanwhile the rest will be destroyed in vy_write_iterator_stop(). I've moved clean-up part from stop() method right to start(). Here's diff: diff --git a/src/box/vy_scheduler.c b/src/box/vy_scheduler.c index 387f58723..9dba93d34 100644 --- a/src/box/vy_scheduler.c +++ b/src/box/vy_scheduler.c @@ -1065,10 +1065,8 @@ vy_task_write_run(struct vy_task *task, bool no_compression) no_compression) != 0) goto fail; - if (wi->iface->start(wi) != 0) { - wi->iface->stop(wi); + if (wi->iface->start(wi) != 0) goto fail_abort_writer; - } int rc; int loops = 0; struct tuple *stmt = NULL; diff --git a/src/box/vy_write_iterator.c b/src/box/vy_write_iterator.c index 21c18d3dc..33ad5ed51 100644 --- a/src/box/vy_write_iterator.c +++ b/src/box/vy_write_iterator.c @@ -401,18 +401,23 @@ vy_write_iterator_start(struct vy_stmt_stream *vstream) struct vy_write_src *src, *tmp; rlist_foreach_entry_safe(src, &stream->src_list, in_src_list, tmp) { if (vy_write_iterator_add_src(stream, src) != 0) - return -1; + goto fail; #ifndef NDEBUG struct errinj *inj = errinj(ERRINJ_VY_WRITE_ITERATOR_START_FAIL, ERRINJ_BOOL); if (inj != NULL && inj->bparam) { inj->bparam = false; diag_set(OutOfMemory, 666, "malloc", "struct vy_stmt"); - return -1; + goto fail; } #endif } return 0; +fail: + /* Clean-up all previously added sources. */ + rlist_foreach_entry_safe(src, &stream->src_list, in_src_list, tmp) + vy_write_iterator_delete_src(stream, src); + return -1; }