From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp29.i.mail.ru (smtp29.i.mail.ru [94.100.177.89]) (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 C0637441840 for ; Tue, 31 Mar 2020 20:44:08 +0300 (MSK) Date: Tue, 31 Mar 2020 17:44:07 +0000 From: Nikita Pettik Message-ID: <20200331174407.GA22927@tarantool.org> References: <8620360c-c99d-f9ea-e7f4-25c97c00c0f1@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <8620360c-c99d-f9ea-e7f4-25c97c00c0f1@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH v2 06/10] box: introduce stacked diagnostic area 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 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.