Tarantool development patches archive
 help / color / mirror / Atom feed
From: Nikita Pettik <korablev@tarantool.org>
To: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Cc: tarantool-patches@dev.tarantool.org
Subject: Re: [Tarantool-patches] [PATCH] box/error: ref error.prev while accessing it
Date: Thu, 16 Apr 2020 12:34:00 +0000	[thread overview]
Message-ID: <20200416123400.GA6229@tarantool.org> (raw)
In-Reply-To: <574107d5-cb85-1b08-3abf-c2c53497ab5e@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

  reply	other threads:[~2020-04-16 12:34 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-04-15 23:36 Nikita Pettik
2020-04-16  0:19 ` Vladislav Shpilevoy
2020-04-16 12:34   ` Nikita Pettik [this message]
2020-04-16 21:17     ` Vladislav Shpilevoy
2020-04-17 20:06       ` Nikita Pettik
2020-04-17 23:58         ` Vladislav Shpilevoy

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200416123400.GA6229@tarantool.org \
    --to=korablev@tarantool.org \
    --cc=tarantool-patches@dev.tarantool.org \
    --cc=v.shpilevoy@tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH] box/error: ref error.prev while accessing it' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox