Re: cvs commit: squid3/lib MemPool.cc

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

Re: cvs commit: squid3/lib MemPool.cc

Amos Jeffries
Administrator
Alex Rousskov wrote:

> rousskov    2007/05/22 10:40:06 MDT
>
>   Modified files:
>     lib                  MemPool.cc
>   Log:
>   Bug #1966 fix: Use rounded String MemPool sizes in the hard-coded pool
>   config to avoid warnings that the configured pool size does not match the
>   actual size.
>  
>   Revision  Changes    Path
>   1.7       +7 -2      squid3/lib/MemPool.cc
>

Did you run make check before committing any of the days changes?

I'm getting 7 of 14 core unit tests fail. Two on MemPool.cc:240, the
rest on silent segfault.


Assertion failed: (aLabel != NULL && aSize) at MemPool.cc:240
FAIL: tests/testString
/bin/bash: line 4: 23339 Aborted                 (core dumped) ${dir}$tst
FAIL: tests/testCacheManager
Assertion failed: (aLabel != NULL && aSize) at MemPool.cc:240
FAIL: tests/testDiskIO
/bin/bash: line 4: 23351 Aborted                 (core dumped) ${dir}$tst
FAIL: tests/testEvent
/bin/bash: line 4: 23358 Aborted                 (core dumped) ${dir}$tst
FAIL: tests/testEventLoop
/bin/bash: line 4: 23371 Aborted                 (core dumped) ${dir}$tst
FAIL: tests/testHttpRequest

Amos
Reply | Threaded
Open this post in threaded view
|

Re: cvs commit: squid3/lib MemPool.cc

Henrik Nordström
tor 2007-05-24 klockan 00:59 +1200 skrev Amos Jeffries:
> >
>
> Did you run make check before committing any of the days changes?
>
> I'm getting 7 of 14 core unit tests fail. Two on MemPool.cc:240, the
> rest on silent segfault.

> Assertion failed: (aLabel != NULL && aSize) at MemPool.cc:240

Hmm...

(gdb) p StrPoolsAttrs
$3 = {{name = 0x509d01 "Short Strings", obj_size = 0}, {name = 0x509d0f
"Medium Strings", obj_size = 0}, {
    name = 0x509d1e "Long Strings", obj_size = 0}}

doesn't look quite right.

Looks like C++ magics strikes again and Mem::Init is called (via the
Initer test class) before StrPoolsAttrs has been assigned..

Mem::Init needs to be called via the setUp() method, not from an
automatic instantiation constructor..


Index: src/tests/testCacheManager.cc
===================================================================
RCS file: /cvsroot/squid/squid3/src/tests/testCacheManager.cc,v
retrieving revision 1.2
diff -u -p -r1.2 testCacheManager.cc
--- src/tests/testCacheManager.cc       18 May 2007 06:41:33 -0000
1.2
+++ src/tests/testCacheManager.cc       23 May 2007 20:47:42 -0000
@@ -17,12 +17,10 @@ shut_down(int)
 
 /* init memory pools */
 
-struct Initer
+void testCacheManager::setUp()
 {
-    Initer() {Mem::Init();}
-};
-
-static Initer ensure_mempools;
+    Mem::Init();
+}
 
 /*
  * Test creating a CacheManager
Index: src/tests/testCacheManager.h
===================================================================
RCS file: /cvsroot/squid/squid3/src/tests/testCacheManager.h,v
retrieving revision 1.1
diff -u -p -r1.1 testCacheManager.h
--- src/tests/testCacheManager.h        29 May 2006 00:15:09 -0000
1.1
+++ src/tests/testCacheManager.h        23 May 2007 20:47:42 -0000
@@ -16,6 +16,7 @@ class testCacheManager : public CPPUNIT_
     CPPUNIT_TEST_SUITE_END();
 
 public:
+    void setUp();
 
 protected:
     void testCreate();


signature.asc (316 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: cvs commit: squid3/lib MemPool.cc

Robert Collins
On Wed, 2007-05-23 at 22:49 +0200, Henrik Nordstrom wrote:
>
> Mem::Init needs to be called via the setUp() method, not from an
> automatic instantiation constructor..

Actually I spent quite some time making it work correctly without setUp.
I think manual setup is very annoying and error prone: if we can avoid
it lets do so.

-Rob
--
GPG key available at: <http://www.robertcollins.net/keys.txt>.

signature.asc (196 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: cvs commit: squid3/lib MemPool.cc

Henrik Nordström
In reply to this post by Henrik Nordström
ons 2007-05-23 klockan 22:49 +0200 skrev Henrik Nordstrom:

> Mem::Init needs to be called via the setUp() method, not from an
> automatic instantiation constructor..

The tests have been updated, and now pass fine here..

Regards
Henrik

signature.asc (316 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: cvs commit: squid3/lib MemPool.cc

Henrik Nordström
In reply to this post by Robert Collins
tor 2007-05-24 klockan 06:58 +1000 skrev Robert Collins:
> On Wed, 2007-05-23 at 22:49 +0200, Henrik Nordstrom wrote:
> >
> > Mem::Init needs to be called via the setUp() method, not from an
> > automatic instantiation constructor..
>
> Actually I spent quite some time making it work correctly without setUp.
> I think manual setup is very annoying and error prone: if we can avoid
> it lets do so.

I disagree.

The initialization should mimic the main program as much as possible. In
squid Mem::Init is called by main(), and setUp() is the closest
equivalence.

Also the two methods is equally manual. Each test needs a list of stuff
to initialize. The only difference is that it's explicitly called by
cppunit and not the compiler. All I did was to remove the static object
which caused the compiler to call the test initialization and moved this
to the test setUp() method instead.

The day Squid is changed to not do manual setup of these things the
tests obviously no longer need (or should) do it either. But that's a
separate question.

Imho doing heavy dependent stuff from static constructors is very
dangerous from the simple fact that you don't know for sure in which
order these is called. It's both link order and compiler dependent, and
for this reason extremely error prone. It's in the area of C++ magics
best avoided. Keep static constructors as simple as possible.

Regards
Henrik

signature.asc (316 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: cvs commit: squid3/lib MemPool.cc

Alex Rousskov
In reply to this post by Amos Jeffries
On Thu, 2007-05-24 at 00:59 +1200, Amos Jeffries wrote:

> Alex Rousskov wrote:
> > rousskov    2007/05/22 10:40:06 MDT
> >
> >   Modified files:
> >     lib                  MemPool.cc
> >   Log:
> >   Bug #1966 fix: Use rounded String MemPool sizes in the hard-coded pool
> >   config to avoid warnings that the configured pool size does not match the
> >   actual size.
> >  
> >   Revision  Changes    Path
> >   1.7       +7 -2      squid3/lib/MemPool.cc
> >
>
> Did you run make check before committing any of the days changes?

Nope. I never do. Will try to remember next time.

Sorry,

Alex.
P.S. Henrik, thanks for fixing the test cases!