Re: [Tarantool-patches] [PATCH v2 06/10] box: introduce stacked diagnostic area

[Nikita Pettik <korablev@tarantool.org>]

On 28 Mar 19:59, Vladislav Shpilevoy wrote:
> Two more comments.
> 
> > diff --git a/test/box/error.test.lua b/test/box/error.test.lua
> > index a0b7d3e78..1fdd6ed98 100644
> > --- a/test/box/error.test.lua
> > +++ b/test/box/error.test.lua
> > @@ -108,4 +108,109 @@ box.error.new(err)
> > +
> >  space:drop()
> 
> 1. You probably need to keep this 'space:drop()' after
> the test related to it.

Isn't it too late?:) I mean test it is related to is finished at
line 20, meanwhile space:drop() is already at line 111 (box/error.test.lua
is already pushed).

> 2. Also I did some double checking if there are no leaks. And came
> up with that test:
> ====================
> diff --git a/extra/exports b/extra/exports
> index 9323996c1..f581026fb 100644
> --- a/extra/exports
> +++ b/extra/exports
> @@ -43,6 +43,7 @@ tnt_iconv_close
>  tnt_iconv
>  exception_get_string
>  exception_get_int
> +get_error_count
>  
>  tarantool_lua_ibuf
>  uuid_nil
> diff --git a/src/lib/core/diag.c b/src/lib/core/diag.c
> index 57da5da44..f5d3c8a18 100644
> --- a/src/lib/core/diag.c
> +++ b/src/lib/core/diag.c
> @@ -31,6 +31,14 @@
>  #include "diag.h"
>  #include "fiber.h"
>  
> +int diag_error_count = 0;
> +
> +int
> +get_error_count(void)
> +{
> +	return __atomic_load_n(&diag_error_count, __ATOMIC_SEQ_CST);
> +}
> +
>  /* Must be set by the library user */
>  struct error_factory *error_factory = NULL;
>  
> @@ -76,6 +84,7 @@ error_create(struct error *e,
>  	     error_f destroy, error_f raise, error_f log,
>  	     const struct type_info *type, const char *file, unsigned line)
>  {
> +	__atomic_add_fetch(&diag_error_count, 1, __ATOMIC_SEQ_CST);
>  	e->destroy = destroy;
>  	e->raise = raise;
>  	e->log = log;
> @@ -92,6 +101,7 @@ error_create(struct error *e,
>  	e->errmsg[0] = '\0';
>  	e->cause = NULL;
>  	e->effect = NULL;
> +
>  }
>  
>  struct diag *
> diff --git a/src/lib/core/diag.h b/src/lib/core/diag.h
> index 665f492fa..bd4ddcf3e 100644
> --- a/src/lib/core/diag.h
> +++ b/src/lib/core/diag.h
> @@ -47,11 +47,16 @@ enum {
>  	DIAG_FILENAME_MAX = 256
>  };
>  
> +extern int diag_error_count;
> +
>  struct type_info;
>  struct error;
>  
>  typedef void (*error_f)(struct error *e);
>  
> +int
> +get_error_count(void);
> +
>  /**
>   * Error diagnostics needs to be equally usable in C and C++
>   * code. This is why there is a common infrastructure for errors.
> @@ -136,6 +141,7 @@ error_unref(struct error *e)
>  		if (cause == NULL)
>  			return;
>  		to_delete = cause;
> +		__atomic_add_fetch(&diag_error_count, -1, __ATOMIC_SEQ_CST);
>  	}
>  }
>  
> diff --git a/test/box/error.test.lua b/test/box/error.test.lua
> index 1fdd6ed98..8f884c372 100644
> --- a/test/box/error.test.lua
> +++ b/test/box/error.test.lua
> @@ -78,6 +78,25 @@ t;
>  
>  test_run:cmd("setopt delimiter ''");
>  
> +ffi = require('ffi')
> +
> +ffi.cdef[[						\
> +	int 						\
> +	get_error_count(void); 				\
> +]]
> +
> +e1 = nil
> +e2 = nil
> +e3 = nil
> +e4 = nil
> +e5 = nil
> +err = nil
> +box.error.clear()
> +collectgarbage('collect')
> +collectgarbage('collect')
> +
> +errcount = ffi.C.get_error_count()
> +
>  -- gh-4778: don't add created via box.error.new() errors to
>  -- Tarantool's diagnostic area.
>  --
> @@ -213,4 +232,20 @@ box.error.set(e1)
>  box.error.clear()
>  assert(e1.prev == e2)
>  
> +e1 = nil
> +e2 = nil
> +e3 = nil
> +e4 = nil
> +e5 = nil
> +err = nil
> +box.error.clear()
> +
> +collectgarbage('collect')
> +collectgarbage('collect')
> +
> +errcount2 = ffi.C.get_error_count()
> +
> +errcount
> +errcount2
> +
>  space:drop()
> ====================
> 
> errcount is 13, errcount2 is 17. Something is probably wrong.
> I thought these were errors created before line

Apllied your diff with one change and every launch I get:

[001] +errcount
[001] + | ---
[001] + | - 1
[001] + | ...
[001] +errcount2
[001] + | ---
[001] + | - 1
[001] + | ...
[001] +

One error I guess is that which gets stuck in diag. The change is:

@@ -124,6 +129,7 @@ error_unref(struct error *e)
                to_delete->cause = NULL;
                to_delete->effect = NULL;
                to_delete->destroy(to_delete);
+               __atomic_add_fetch(&diag_error_count, -1, __ATOMIC_SEQ_CST);
                if (cause == NULL)
                        return;
                to_delete = cause;
 
I.e. we should accout destroy before checking cause since for
the last error in the list it is NULL, ergo it won't be accounted.

Nit: could you please send diff as a separate mail attachment next time?
The thing is I have to manually extract it to a separate file
instead of being capable of applying it as a patch via git am/apply.
Thanks.

>     gh-4778: don't add created via box.error.new() errors to
> 
> But the difference still should be 0. Because I nullified and
> cleared all variables you use before and after the test. I tried
> commenting out all the code before this line, and still get a
> not 0 difference. I ask you to check why some errors are not
> deleted.