[Asterisk-code-review] Add support for delayed shutdown of Asterisk when checking f... (testsuite[master])

[George Joseph]

George Joseph has posted comments on this change. ( https://gerrit.asterisk.org/6845 )

Change subject: Add support for delayed shutdown of Asterisk when checking for leaks.
......................................................................


Patch Set 3:

> > I'm not a real fan of having to add a parameter to every test's
 > > yaml.  How about making this a command line option that
 > > automatically adds
 > > 'n' amount to very reactor timeout?
 > >
 > > --reactor-timeout-adjustment=5 to add 5 seconds, or if you have a
 > > really fast machine,  --reactor-timeout-adjustment=-5 to reduce
 > the
 > > timeout by 5 seconds.
 > 
 > I think you're misunderstanding the purpose of this patch.  Reactor
 > timeout is a side-effect, not the main point.  Reactor timeout is
 > only touched to deal with the case where a test needs >30 second
 > shutdown delay.  So if we have memcheck-delay-stop: 90 to deal with
 > a minse timer we increase the reactor-timeout to 93 seconds minimum
 > to avoid showing reactor timeout messages while the test is
 > stopping.
 > 
 > This patch is not intended to deal with any slowness caused by
 > valgrind or REF_DEBUG (code to do that globally is already in
 > place).  The point is to wait long enough for any relevant timers
 > to fire and cleanup resources.  SIP timer_b takes just as long to
 > fire with or without memory checking, the only difference is that
 > we care enough to wait for it when we are checking memory.
 > 
 > In my own testing this memcheck-delay-stop will only be needed on
 > about half the PJSIP tests and is unneeded by nearly all non-PJSIP
 > tests.  Many of the PJSIP tests will need 7 seconds (assuming
 > timer_b is configured to minimum).  Some tests will need 60 seconds
 > (ast_sip_session_defer_termination), others might need 90 seconds
 > (minse configured to minimum).  I had considered setting the
 > default memcheck-delay-stop to 7 seconds but this would have
 > drawbacks.  A 7 second delay for every test would increase the
 > run-time of the full testsuite by nearly 2 hours (1000 tests). 
 > Maybe 200 of the tests need any delay.  Most cases we're dealing
 > with timer_b which means that we need to reconfigured timer_t1 +
 > timer_b or use memcheck-delay-stop: 32, so for a 7 second delay to
 > ever be effective we'd need to touch the pjsip.conf files anyways.

Ok, can you update the commit message to explain all that.


-- 
To view, visit https://gerrit.asterisk.org/6845
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-Project: testsuite
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I67ec89e4a116526ad34796a233e15fec794cf3e1
Gerrit-Change-Number: 6845
Gerrit-PatchSet: 3
Gerrit-Owner: Corey Farrell <git at cfware.com>
Gerrit-Reviewer: Corey Farrell <git at cfware.com>
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Jenkins2
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
Gerrit-Comment-Date: Mon, 30 Oct 2017 16:16:23 +0000
Gerrit-HasComments: No
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20171030/549fcf5e/attachment.html>