BIND 10 #554: Abstract UDPQuery to IOFetch which will be transport layer agnostic

[BIND 10 Development]

#554: Abstract UDPQuery to IOFetch which will be transport layer agnostic
-------------------------------------+-------------------------------------
                 Reporter:  smann    |                Owner:  jelte
                     Type:           |               Status:  reviewing
  enhancement                        |            Milestone:  R-Team-
                 Priority:  major    |  Sprint-20110222
                Component:           |           Resolution:
  resolver                           |            Sensitive:  0
                 Keywords:           |  Add Hours to Ticket:  0
  UDPQuery fetch udp tcp             |          Total Hours:  0
Estimated Number of Hours:  8.0      |
                Billable?:  1        |
                Internal?:  0        |
-------------------------------------+-------------------------------------
Changes (by stephen):

 * owner:  stephen => jelte


Comment:

 > io_asio_socket.h: (documentation) if this is a no-op on UDP sockets,
 does that mean that the callback is never called,
 > or always immediately called? (doc is a bit unclear on that, perhaps we
 could also use some examples if we decide to
 > keep this)
 I've beefed up the header documentation here.


 > io_completion_cb.h: just wondering if we should make it clear in the
 name of this class that it itself is an intention nop
 Changed the class name to DummyIOCallback.  Originally it was the type of
 the callback objects passed to the asynchronous I/O methods (and the base
 class of IOFetch), but the name never got changed when the type was made a
 template argument.

 > io_fetch.h: I mentioned this quickly on our call last week, I suspect
 that this constructor for IOFetchData
 > will not suffice. It's not really necessary now, but i think having the
 Question as its main 'what exactly
 > am i asking here' may not be enough (we should probably either go for a
 full Message ref, or the other
 > way around, an already rendered buffer of data). As i am not sure which
 would be the better option
 > (also depends on what we get from the NSAS, and how we will handle auths
 wanting their data in a specific
 > way (and fallbacks for that), I think keeping this in mind as a TODO is
 good enough for now. Just
 > wanted to mention it :)
 I've put in a TODO, but see below concerning the refactoring of ASIO.


 > I don't get the comment
 >
 >   The default constructor and copy constructor are correct for this
 method.
 Hangover from an earlier attempt to get it working, now removed.




 > io_fetch.cc:
 >  Say, is any function guaranteed to finish completely before more events
 can happen (even if it potentially
 > calls new i/o)? Otherwise i'd move the line
 > 173: data_->stopped = true;
 > up a bit.
 In a single-threaded environment, the callback functions ''should''
 execute sequentially, but I've moved it to the start of the "if" block,
 just in case this is not true.  In the longer term, the documentation
 indicates that you can have many threads servicing a single I/O queue I've
 also added a "TODO" to note that if we go multi-threaded, we will need to
 test and set stopped_ inside a mutex.


 > tcp_socket.h:
 > 42: / Other notes about TCPSocket applies to this class too
 > er, what notes is this referring to?
 Comment removed.

 > 54: MAX_SIZE = 4096.
 > This isn't actually used, is it?
 Now removed.

 > udp_server.cc:
 > mention of TCP socket class in doxygen docs, i assume this should say
 UDP socket class.
 Done.

 > General question; should the TCP version of fetch also support queries
 that result in multiple answer
 > packets? We do not need them in the resolver, but we would need them for
 xfrin and an equivalent
 > of dig.
 In current version of the code, IOFetch is a packet exchange - the looping
 within IOFetch for TCP was purely to cope with the case where the logical
 packet of data (as defined by the count field in the TCP stream) is split
 and requires multiple receive operations to acquire it.

 When I do ticket 499 (adding TCP support), I'll make it a message exchange
 - have the user supply a callback to inspect each packet and to return an
 indication as to whether the entire message has been received.

 As it stands, IOFetch doesn't care what's in the data. (OK, it does a bit
 in that given a question it constructs a DNS message to send to the remote
 system.  But as you suggest above, it could be an already rendered buffer
 of data, in which case it would be truly content-unaware.) Adding a
 message completion callback to the code retains that abstraction and seems
 the easiest way to achieve that requirement.

 When we refactor the code we should look again at this question.  In
 particular, I think we sahould ensure that the asiolink code contains no
 reference to either the resolver library or the DNS library - it should be
 purely about exchanging data with a remove server.


 > Oh, and don't forget to revert that INSTALL file :)
 I haven't forgotten :-)

 Changes checked in as commit 6dad3efd9d023633d973d20b9102f2f8b10f3d17

-- 
Ticket URL: <https://bind10.isc.org/ticket/554#comment:5>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development