BIND 10 #1175: find another solution to synchronize between threads used in unittests for stats

[BIND 10 Development]

#1175: find another solution to synchronize between threads used in unittests for
stats
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  naokikambe
  naokikambe                         |                Status:  reviewing
                       Type:         |             Milestone:
  defect                             |  Sprint-20110927
                   Priority:  major  |            Resolution:
                  Component:         |             Sensitive:  0
  statistics                         |           Sub-Project:  DNS
                   Keywords:         |  Estimated Difficulty:  5
            Defect Severity:  N/A    |           Total Hours:  0
Feature Depending on Ticket:         |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------
Changes (by vorner):

 * owner:  vorner => naokikambe


Comment:

 Hello

 Indeed the patch did contain many changes not directly or obviously
 related to the topic of the ticket. While I agree they are useful, they
 could have been potentially separated under a different ticket. Anyway, no
 need to do anything about it now, as I got through them already.

 So, some comments and questions:
  * This part returs if there are no listen addresses configured or if the
 binding failed. Why is it so? What is the purpose of the code? Shouldn't
 it fail if it can't bind? (Not that I'd say it's wrong, I just don't
 really understand)
 {{{#!python
         # If the http sockets aren't opened or
         # if new_config doesn't have'listen_on', it returns
         if len(self.httpd) == 0 or 'listen_on' not in new_config:
             return isc.config.ccsession.create_answer(0)
         self.close_httpd()
 }}}
  * The description here might need some improvement. It's not clear that
 the port is first one tried and it goes up until it finds one that's free.
 {{{#!python
 def get_availaddr(address='127.0.0.1', port=8001):
     """returns tuple of address and port available to listen on the
     platform. Default port range is between 8001 and 65535. If port is
     over flow(greater than 65535), OverflowError is thrown"""
 }}}
  * Is this really needed? Isn't it enough for the garbage collector to
 claim and close the socket? (your way might be cleaner anyway, but I'm
 interested)
 {{{#!python
         finally:
             if sock: sock.close()
 }}}
  * This is not related to this branch, but I noticed you're sending
 absolute redirects. Would relative ones be better? For example if there's
 a redirected port in address translator, the absolute redirect may be
 wrong:
 {{{
                           self.assertEqual(response.getheader('Location'),
 -                         "http://%s:%d%s" % (address, port,
 stats_httpd.XML_URL_PATH))
 +                         "http://%s:%d%s" % (self.address, self.port,
 stats_httpd.XML_URL_PATH))
 }}}
  * Aestetical: two hash signs
 {{{#!python
 # # 404 NotFound
 }}}
  * You set up the SIGALRM in `setUp` and call `signal.alarm` to schedule
 one. That is OK to stop a test that run for too long. But you don't cancel
 the alarm in tearDown, so if there's bunch of long-running tests some time
 after that, the alarm may come to a wrong test (if that test does not set
 its own alarm). That would make a completely different test fail. I
 suggest that you properly cancel the alarm in `tearDown` to prevent any
 possible future surprises.
  * This code looks strange:
 {{{#!python
 # dual stack (address is ipv4)
 if self.ipv6_enabled:
         server_addresses = get_availaddr()
         self.stats_httpd = MyStatsHttpd(server_addresses)
         for ht in self.stats_httpd.httpd:
                 self.assertTrue(isinstance(ht, stats_httpd.HttpServer))
                 self.assertEqual(ht.address_family, socket.AF_INET)
                 self.assertTrue(isinstance(ht.socket, socket.socket))

 # only-ipv4 single stack
 if not self.ipv6_enabled:
         server_addresses = get_availaddr()
         self.stats_httpd = MyStatsHttpd(server_addresses)
         for ht in self.stats_httpd.httpd:
                 self.assertTrue(isinstance(ht, stats_httpd.HttpServer))
                 self.assertEqual(ht.address_family, socket.AF_INET)
                 self.assertTrue(isinstance(ht.socket, socket.socket))
 }}}
    For one, the two conditions look complement to each other, therefore an
 if-else might look more natural. But what surprises me more is that both
 branches look the same. That is confusing, because then everyone who reads
 the code spends some time trying to figure out why there are two versions
 and what is the trick. So, unless there actually is a trick I did not
 notice, I think it would be better to simplify it and have only one copy
 without the conditions.
  * Talking about `is_ipv6_enabled` ‒ why isn't `socket.has_ipv6` enough?
  * `test_select_failure2` ‒ does that test that the code does _not_ fail?
 If so, maybe a better name or comment could be used.
  * This looks scary. I think this would deserve a comment why the retries
 are necessary.
 {{{#!python
     def __init__(self, server, *args, **kwargs):
         self.server = None
         n = 0
         while True:
             try:
                 self.server = server(*args, **kwargs)
             except isc.cc.session.SessionTimeout:
                 if self.server is not None:
                     self.server.shutdown()
                 # retrying until 3 times
                 if n >2: raise
                 n = n + 1
                 continue
             else: break
 }}}

-- 
Ticket URL: <http://bind10.isc.org/ticket/1175#comment:12>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development