From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-lf1-f65.google.com (mail-lf1-f65.google.com [209.85.167.65]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 1952046970F for ; Wed, 27 Nov 2019 11:41:06 +0300 (MSK) Received: by mail-lf1-f65.google.com with SMTP id y5so3732816lfy.7 for ; Wed, 27 Nov 2019 00:41:05 -0800 (PST) Date: Wed, 27 Nov 2019 11:41:04 +0300 From: Konstantin Osipov Message-ID: <20191127084104.GD2752@atlas> References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Subject: Re: [Tarantool-patches] [PATCH 2/2] func: fix not unloading of unused modules List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Vladislav Shpilevoy Cc: tarantool-patches@dev.tarantool.org * Vladislav Shpilevoy [19/11/27 11:15]: > C functions are loaded from .so/.dylib dynamic libraries. A > library is loaded when any function from there is called first > time. And was supposed to be unloaded, when all its functions are > dropped from the schema (box.schema.func.drop()), and none of them > is still in a call. But the unloading part was broken. Wow. This is damn genius! It never occurred to me that error injections could be used to test some obscure code paths. The impact of this on testing is going to be huge. > module_gc(struct module *module) > { > - if (!module->is_unloading || !rlist_empty(&module->funcs) || > - module->calls != 0) > - return; > - module_delete(module); > + if (rlist_empty(&module->funcs) && module->calls == 0) > + module_delete(module); > } This is LGTM. Georgy wrote the original patches so you may want to solicit his review as well. > +f1:wakeup() > + | --- > + | ... > +test_run:wait_cond(function() return f1:status() == 'dead' end) > + | --- > + | - true > + | ... > +check_module_count_diff(0) > + | --- > + | ... > + > +f2:wakeup() This is a bit hackish way to cancel a sleep. I thought we had errinj_sleep injection which would reliably cancel a sleep when you clear it? If there is no such function I don't think it is a blocker for this patch to get in as is. -- Konstantin Osipov, Moscow, Russia