BIND 10 #1784: b10-resolver incorrectly uses SyncUDPServer

BIND 10 Development do-not-reply at isc.org
Fri Mar 16 23:46:07 UTC 2012


#1784: b10-resolver incorrectly uses SyncUDPServer
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  jinmei
  jinmei                             |                Status:  accepted
                       Type:         |             Milestone:
  defect                             |  Sprint-20120320
                   Priority:  very   |            Resolution:
  high                               |             Sensitive:  0
                  Component:         |           Sub-Project:  DNS
  resolver                           |  Estimated Difficulty:  0
                   Keywords:         |           Total Hours:  0
            Defect Severity:  N/A    |
Feature Depending on Ticket:         |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 trac1784 is ready for review.

 The main fix is essentially a few lines of changes to the main code,
 but it also contains fix to a number of issues in the original
 #1600 implementation:

 - the original version almost totally lacked necessary unit tests.
   the almost only tests were for an important case and are for
   the part of code that is not really used by the main implementation.
 - the original version had almost no documentation for the added
   interface.

 If at least either of an these were attempted, we may have been able
 to notice this problem before merge.  And, as a related issue:

 - the original version updated the part of the code that is not
   necessary for the purpose of this task; in fact, it updated the
   code that is not really used in any bind10 application,
   specifically, some mostly deprecated methods of DNSService.
   (I'm not sure we should keep it, but that's a separate question),
   and, these were not tested at all.

 This is a related issue of missing tests or documentation.  If we
 diligently try to write tests and documentation, it would give us
 an opportunity of reconsidering whether the addition is really
 necessary.  If we allow the developer to add something without
 thinking about it much, that often leads to bugs like this.

 Anyway, I've fixed all these issues: fix the main problem, added
 detailed tests and documentation, and removed the dead addition.
 I've also updated the interface a bit; that's in some sense a matter
 of preference, but I saw the original one was quite confusing.  Also,
 its default was bad - the default must be something most general,
 while the default of the previous version is for optimization.  That's
 another background cause of this bug.

 For testing I've introduced an inheritance into the DNSService class.
 That made the changes larger, but I believe it's a cleaner solution.
 I guess we'll need something like MockDNSSerivce for other purposes,
 too.  Also, thanks to this abstraction we can now get rid of the
 ugly "test_mode" global variable from portconfig (so I did that too).

 Finally, I renamed asiodns/tests/io_service_unittest.cc to
 dns_service_unittest.cc because that's more appropriate in terms of
 what's tested in that file.  That makes the diff even bigger, but
 the essentially the only change is the lines below the introduction of
 the helper TestLookup class.

 I don't think we need a changelog for this.

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


More information about the bind10-tickets mailing list