From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng1.m.smailru.net (smtpng1.m.smailru.net [94.100.181.251]) (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 92D3A469719 for ; Thu, 15 Oct 2020 01:40:38 +0300 (MSK) References: <20201014132939.GA173841@grain> From: Vladislav Shpilevoy Message-ID: Date: Thu, 15 Oct 2020 00:40:36 +0200 MIME-Version: 1.0 In-Reply-To: <20201014132939.GA173841@grain> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Tarantool-patches] [PATCH 2/6] raft: factor out the code to wakeup worker fiber List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Cyrill Gorcunov Cc: tarantool-patches@dev.tarantool.org Hi! Thanks for the review! On 14.10.2020 15:29, Cyrill Gorcunov wrote: > On Wed, Oct 14, 2020 at 01:28:28AM +0200, Vladislav Shpilevoy wrote: >> -raft_schedule_broadcast(void) >> +raft_worker_wakeup(void) >> { >> - raft.is_broadcast_scheduled = true; >> + if (raft.worker == NULL) { >> + raft.worker = fiber_new("raft_worker", raft_worker_f); >> + fiber_set_joinable(raft.worker, true); >> + } > > When fiber_new return NULL you'll get nil dereference in fiber_set_joinable. Indeed, you are right! It seems it was there always. There is no a beautiful way to fix it now, so I added a panic() here in case of OOM: ==================== --- a/src/box/raft.c +++ b/src/box/raft.c @@ -1010,6 +1010,21 @@ raft_worker_wakeup(void) { if (raft.worker == NULL) { raft.worker = fiber_new("raft_worker", raft_worker_f); + if (raft.worker == NULL) { + /* + * XXX: should be handled properly, no need to panic. + * The issue though is that most of the Raft state + * machine functions are not supposed to fail, and also + * they usually wakeup the fiber when their work is + * finished. So it is too late to fail. On the other + * hand it looks not so good to create the fiber when + * Raft is initialized. Because then it will occupy + * memory even if Raft is not used. + */ + diag_log(); + panic("Could't create Raft worker fiber"); + return; + } fiber_set_joinable(raft.worker, true); } /*