Re: [Tarantool-patches] [PATCH] box/error: ref error.prev while accessing it

[Nikita Pettik <korablev@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