From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp16.mail.ru (smtp16.mail.ru [94.100.176.153]) (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 E749F4696C3 for ; Thu, 16 Apr 2020 15:34:01 +0300 (MSK) Date: Thu, 16 Apr 2020 12:34:00 +0000 From: Nikita Pettik Message-ID: <20200416123400.GA6229@tarantool.org> References: <3ee69eaa4ff23647e9ae6fe63e2ae77c427ffe01.1586993218.git.korablev@tarantool.org> <574107d5-cb85-1b08-3abf-c2c53497ab5e@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <574107d5-cb85-1b08-3abf-c2c53497ab5e@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH] box/error: ref error.prev while accessing it List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Vladislav Shpilevoy Cc: tarantool-patches@dev.tarantool.org On 16 Apr 02:19, Vladislav Shpilevoy wrote: > Hi! Thanks for the patch! > > See 2 minor comments below. > > > diff --git a/src/lib/core/diag.h b/src/lib/core/diag.h > > index 7a5454d1c..beb6a7528 100644 > > --- a/src/lib/core/diag.h > > +++ b/src/lib/core/diag.h > > @@ -118,25 +118,8 @@ error_ref(struct error *e) > > e->refs++; > > } > > > > -static inline void > > -error_unref(struct error *e) > > 1. To reduce diff size and to keep inlining when > possible you can remove word 'static', and add > > extern inline void > error_unref(struct error *e); > > to diag.c. error_unref() doesn't seem to be small enough to keep it inlining after diagnostic area introduction, so I'd better remove inline attribute anyway. > Yeah, exactly 'extern inline', not just 'extern'. > I recently discovered that it is a special thing for > such cases. Thanks, will keep it in mind. > > diff --git a/test/box/error.result b/test/box/error.result > > index df6d0ebc0..49e35b942 100644 > > --- a/test/box/error.result > > +++ b/test/box/error.result > > @@ -831,3 +831,51 @@ assert(box.error.last() == e1) > > + | ... > > +err.prev > > + | --- > > + | - null > > + | ... > > +preve > > + | --- > > + | - '[string "return function(tuple) local json = require(''..."]:1: attempt to call > > + | global ''require'' (a nil value)' > > + | ... > > + > > +s:drop() > > + | --- > > + | ... > > +box.schema.func.drop('runtimeerror') > > 2. Did you check that preve does not leak when GC starts working? > You can use setmetatable({}, {__mode = 'v'}) for that, see > examples by grepping " __mode = 'v' ". Yep, I verified it using prints since I didn't know about weak references in Lua. Thanks, I've extended test. Diff: diff --git a/test/box/error.result b/test/box/error.result index 49e35b942..ee9dc2a85 100644 --- a/test/box/error.result +++ b/test/box/error.result @@ -860,6 +860,9 @@ ok, err = test_func() preve = err.prev | --- | ... +gc_err = setmetatable({preve}, {__mode = 'v'}) + | --- + | ... err:set_prev(nil) | --- | ... @@ -867,10 +870,27 @@ err.prev | --- | - null | ... -preve +collectgarbage('collect') + | --- + | - 0 + | ... +-- Still one reference to err.prev so it should not be collected. +-- +gc_err + | --- + | - - '[string "return function(tuple) local json = require(''..."]:1: attempt to call + | global ''require'' (a nil value)' + | ... +preve = nil + | --- + | ... +collectgarbage('collect') + | --- + | - 0 + | ... +gc_err | --- - | - '[string "return function(tuple) local json = require(''..."]:1: attempt to call - | global ''require'' (a nil value)' + | - [] | ... s:drop() diff --git a/test/box/error.test.lua b/test/box/error.test.lua index dbc4d143f..3ae197379 100644 --- a/test/box/error.test.lua +++ b/test/box/error.test.lua @@ -242,9 +242,16 @@ idx = s:create_index('idx', {func = box.func.runtimeerror.id, parts = {{1, 'stri function test_func() return pcall(s.insert, s, {1}) end ok, err = test_func() preve = err.prev +gc_err = setmetatable({preve}, {__mode = 'v'}) err:set_prev(nil) err.prev -preve +collectgarbage('collect') +-- Still one reference to err.prev so it should not be collected. +-- +gc_err +preve = nil +collectgarbage('collect') +gc_err