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:  vorner
  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 naokikambe):

 * owner:  naokikambe => vorner


Comment:

 Replying to [comment:12 vorner]:

 Hello, thank you for comments and questions. Please note that all of the
 following commits are for the new branch "trac1175_reapplied".

 > 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.

 That's right, but I'm sorry for the confusion. If the separation is still
 required, I will do that.

 >  * 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)

 Because it doesn't need to do so. It doesn't need to close the httpd
 socket if no http socket is opened yet. Normally it is opened by the
 open_httpd() method in __init__(). It doesn't need to reopen the existent
 http socket unless the listen_on config is changed. Please see also
 https://lists.isc.org/mailman/htdig/bind10-dev/2011-August/002582.html.

 >  * 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.

 I tried to revise it on git 6f4ae07aad42cb8cf474d35c4459e42e126556b3.
 Please see it.

 >  * 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)

 If it successfully finds the available address and port, it needs to close
 it for the reuse before it returns. I don't think the garbage collector
 might do that enough on any environment. Is that correct?

 >  * 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:

 It uses a "Location" header in HTTP for the redirection. A absolute URI
 seems to be always required for the value of the header according to the
 section 14.30 in RFC 2616.

 >  * Aestetical: two hash signs

 Thank you! I fixed on git 21358e1c1b8d3027ae10e3da656dc351fd17bfae.

 >  * 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.

 That's right. I created a new class for that. Please see git
 2d07ef93c566cc2d801bc9d3c0d55d5794bbc102.

 >  * This code looks strange:
 >    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.

 I removed them. Please see git a881def076ce58f3a46303094b4bffd51f24b1b4.

 >  * Talking about `is_ipv6_enabled` - why isn't `socket.has_ipv6` enough?

 Because the value was True even though I disabled IPv6 config on my
 environment.

 >  * `test_select_failure2` - does that test that the code does _not_
 fail? If so, maybe a better name or comment could be used.

 I revised the names and comments of the functions. Please see git
 9a77593f1015bbbe7bca2e95e7b20a18d66a9f0f.

 >  * This looks scary. I think this would deserve a comment why the
 retries are necessary.

 I revised the comment but this retry may not be so important. Please see
 git cef9bd05810891bb4d0b44f0dc3ad47ee8161784.

 Best

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