[Tarantool-patches] [PATCH] box/error: ref error.prev while accessing it
Nikita Pettik
korablev at tarantool.org
Thu Apr 16 15:34:00 MSK 2020
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
More information about the Tarantool-patches
mailing list