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